From bb6377e679dbb6e758af98fc0b65b12cf1f40b34 Mon Sep 17 00:00:00 2001 From: Janis Danisevskis Date: Fri, 23 Feb 2018 11:23:20 -0800 Subject: [PATCH] Some changes required for the actual integration with a GUI renderer These changes accumulated during the integration with the Pixel specific impelemtation. The make it easiser to integrate an GUI renderer with the core logic. Bug: 63928580 Test: VTS tests and manual tests Change-Id: I7001f60709ce806a16f098492bdb71eb05e6ca9a --- confirmationui/1.0/default/ConfirmationUI.cpp | 7 ++- .../1.0/default/PlatformSpecifics.h | 10 +++- .../1.0/generic/GenericOperation.h | 46 +++++++++++++------ 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/confirmationui/1.0/default/ConfirmationUI.cpp b/confirmationui/1.0/default/ConfirmationUI.cpp index f241a76096..41e03ce80a 100644 --- a/confirmationui/1.0/default/ConfirmationUI.cpp +++ b/confirmationui/1.0/default/ConfirmationUI.cpp @@ -43,7 +43,12 @@ Return ConfirmationUI::promptUserConfirmation( const hidl_vec& extraData, const hidl_string& locale, const hidl_vec& uiOptions) { auto& operation = MyOperation::get(); - return operation.init(resultCB, promptText, extraData, locale, uiOptions); + auto result = operation.init(resultCB, promptText, extraData, locale, uiOptions); + if (result == ResponseCode::OK) { + // This is where implementation start the UI and then call setPending on success. + operation.setPending(); + } + return result; } Return ConfirmationUI::deliverSecureInputEvent( diff --git a/confirmationui/1.0/default/PlatformSpecifics.h b/confirmationui/1.0/default/PlatformSpecifics.h index 18b88c8fcc..488da6d1b7 100644 --- a/confirmationui/1.0/default/PlatformSpecifics.h +++ b/confirmationui/1.0/default/PlatformSpecifics.h @@ -52,8 +52,14 @@ class HMacImplementation { const uint8_t key[32], std::initializer_list buffers); }; -using MyOperation = generic::Operation, MonotonicClockTimeStamper, - HMacImplementation>; +class MyOperation : public generic::Operation, + MonotonicClockTimeStamper, HMacImplementation> { + public: + static MyOperation& get() { + static MyOperation op; + return op; + } +}; } // namespace implementation } // namespace V1_0 diff --git a/confirmationui/support/include/android/hardware/confirmationui/1.0/generic/GenericOperation.h b/confirmationui/support/include/android/hardware/confirmationui/1.0/generic/GenericOperation.h index a88cd40e32..b480942998 100644 --- a/confirmationui/support/include/android/hardware/confirmationui/1.0/generic/GenericOperation.h +++ b/confirmationui/support/include/android/hardware/confirmationui/1.0/generic/GenericOperation.h @@ -55,10 +55,27 @@ class Operation { (void)uiOptions; resultCB_ = resultCB; if (error_ != ResponseCode::Ignored) return ResponseCode::OperationPending; - // TODO make copy of promptText before using it may reside in shared buffer - auto state = write( - WriteState(formattedMessageBuffer_), - map(pair(text("prompt"), text(promptText)), pair(text("extra"), bytes(extraData)))); + + // We need to access the prompt text multiple times. Once for formatting the CBOR message + // and again for rendering the dialog. It is vital that the prompt does not change + // in the meantime. As of this point the prompt text is in a shared buffer and therefore + // susceptible to TOCTOU attacks. Note that promptText.size() resides on the stack and + // is safe to access multiple times. So now we copy the prompt string into the + // scratchpad promptStringBuffer_ from where we can format the CBOR message and then + // pass it to the renderer. + if (promptText.size() >= uint32_t(MessageSize::MAX)) + return ResponseCode::UIErrorMessageTooLong; + auto pos = std::copy(promptText.c_str(), promptText.c_str() + promptText.size(), + promptStringBuffer_); + *pos = 0; // null-terminate the prompt for the renderer. + + // Note the extra data is accessed only once for formating the CBOR message. So it is safe + // to read it from the shared buffer directly. Anyway we don't trust or interpret the + // extra data in any way so all we do is take a snapshot and we don't care if it is + // modified concurrently. + auto state = write(WriteState(formattedMessageBuffer_), + map(pair(text("prompt"), text(promptStringBuffer_, promptText.size())), + pair(text("extra"), bytes(extraData)))); switch (state.error_) { case Error::OK: break; @@ -71,20 +88,20 @@ class Operation { return ResponseCode::Unexpected; } formattedMessageLength_ = state.data_ - formattedMessageBuffer_; - // setup TUI and diagnose more UI errors here. + // on success record the start time startTime_ = TimeStamper::now(); if (!startTime_.isOk()) { return ResponseCode::SystemError; } - error_ = ResponseCode::OK; return ResponseCode::OK; } + void setPending() { error_ = ResponseCode::OK; } + void setHmacKey(const uint8_t (&key)[32]) { hmacKey_ = {key}; } void abort() { - // tear down TUI here if (isPending()) { resultCB_->result(ResponseCode::Aborted, {}, {}); error_ = ResponseCode::Ignored; @@ -92,7 +109,6 @@ class Operation { } void userCancel() { - // tear down TUI here if (isPending()) error_ = ResponseCode::Canceled; } @@ -104,10 +120,10 @@ class Operation { } bool isPending() const { return error_ != ResponseCode::Ignored; } - - static Operation& get() { - static Operation operation; - return operation; + const hidl_string getPrompt() const { + hidl_string s; + s.setToExternal(promptStringBuffer_, strlen(promptStringBuffer_)); + return s; } ResponseCode deliverSecureInputEvent(const HardwareAuthToken& secureInputToken) { @@ -156,7 +172,6 @@ class Operation { return result; } hidl_vec userConfirm(const uint8_t key[32]) { - // tear down TUI here if (error_ != ResponseCode::OK) return {}; confirmationTokenScratchpad_ = HMacer::hmac256(key, "confirmation token", getMessage()); if (!confirmationTokenScratchpad_.isOk()) { @@ -169,9 +184,10 @@ class Operation { return result; } - ResponseCode error_; + ResponseCode error_ = ResponseCode::Ignored; uint8_t formattedMessageBuffer_[uint32_t(MessageSize::MAX)]; - size_t formattedMessageLength_; + char promptStringBuffer_[uint32_t(MessageSize::MAX)]; + size_t formattedMessageLength_ = 0; NullOr> confirmationTokenScratchpad_; Callback resultCB_; typename TimeStamper::TimeStamp startTime_;