LogonUI uses fixed buffer size to pass the json object to rundll32. To fix the...

LogonUI uses fixed buffer size to pass the json object to rundll32. To fix the issue, first write/read the buffer size to/from the pipe.

Bug: 902911 https://bugs.chromium.org/p/chromium/issues/detail?id=902911
Change-Id: Ibfc23a098c53c4b6c58a979ee38fb46dc38d55bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1825909
Commit-Queue: Emre Kanlikilicer <emreknlk@google.com>
Reviewed-by: Tien Mai <tienmai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700446}

chrome/credential_provider/gaiacp/dllmain.cc

@@ -160,26 +160,41 @@ void CALLBACK SaveAccountInfoW(HWND /*hwnd*/,
     return;
   }
 
-  char buffer[credential_provider::CGaiaCredentialBase::kAccountInfoBufferSize];
-  DWORD buffer_len_bytes = static_cast<DWORD>(sizeof(buffer));  // In bytes.
-  if (!::ReadFile(hStdin, buffer, buffer_len_bytes, &buffer_len_bytes,
+  // First, read the buffer size.
+  DWORD buffer_size = 0;
+  DWORD bytes_read = 0;
+  if (!::ReadFile(hStdin, &buffer_size, sizeof(buffer_size), &bytes_read,
                   nullptr)) {
     HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
+    LOGFN(ERROR) << "ReadFile for buffer size failed. hr=" << putHR(hr);
+    return;
+  }
+
+  // For security, we check for a max of 1 MB buffer size.
+  const DWORD kMaxBufferSizeAllowed = 1024 * 1024;  // 1MB
+  if (!buffer_size || buffer_size > kMaxBufferSizeAllowed) {
+    LOGFN(ERROR) << "Invalid buffer size.";
+    return;
+  }
+
+  // Second, read the buffer.
+  std::vector<char> buffer(buffer_size, 0);
+  if (!::ReadFile(hStdin, buffer.data(), buffer.size(), &bytes_read, nullptr)) {
+    HRESULT hr = HRESULT_FROM_WIN32(::GetLastError());
     LOGFN(ERROR) << "ReadFile hr=" << putHR(hr);
     return;
   }
-  buffer[buffer_len_bytes] = 0;
   // Don't log |buffer| since it contains sensitive info like password.
 
   HRESULT hr = S_OK;
   base::Optional<base::Value> properties =
-      base::JSONReader::Read(buffer, base::JSON_ALLOW_TRAILING_COMMAS);
+      base::JSONReader::Read(buffer.data(), base::JSON_ALLOW_TRAILING_COMMAS);
 
-  credential_provider::SecurelyClearBuffer(buffer, base::size(buffer));
+  credential_provider::SecurelyClearBuffer(buffer.data(), buffer.size());
 
   if (!properties || !properties->is_dict()) {
-    LOGFN(ERROR) << "base::JSONReader::Read failed length=" << buffer_len_bytes;
-    hr = E_FAIL;
+    LOGFN(ERROR) << "base::JSONReader::Read failed length=" << buffer.size();
+    return;
   }
 
   hr = credential_provider::CGaiaCredentialBase::SaveAccountInfo(*properties);

chrome/credential_provider/gaiacp/gaia_credential_base.cc

@@ -1698,12 +1698,20 @@ HRESULT CGaiaCredentialBase::ForkSaveAccountInfoStub(const base::Value& dict,
   // Write account info to stdin of child process.  This buffer is read by
   // SaveAccountInfoW() in dllmain.cpp.  If this fails, chrome won't pick up
   // the credentials from the credential provider and will need to sign in
-  // manually.  TODO(crbug.com/902911): Figure out how to handle this.
+  // manually.
   std::string json;
   if (base::JSONWriter::Write(dict, &json)) {
-    DWORD written;
-    if (!::WriteFile(parent_handles.hstdin_write.Get(), json.c_str(),
-                     json.length() + 1, &written, /*lpOverlapped=*/nullptr)) {
+    const DWORD buffer_size = json.length() + 1;
+    LOGFN(INFO) << "Json size: " << buffer_size;
+
+    DWORD written = 0;
+    // First, write the buffer size then write the buffer content.
+    if (!::WriteFile(parent_handles.hstdin_write.Get(), &buffer_size,
+                     sizeof(buffer_size), &written, /*lpOverlapped=*/nullptr)) {
+      HRESULT hrWrite = HRESULT_FROM_WIN32(::GetLastError());
+      LOGFN(ERROR) << "WriteFile hr=" << putHR(hrWrite);
+    } else if (!::WriteFile(parent_handles.hstdin_write.Get(), json.c_str(),
+                            buffer_size, &written, /*lpOverlapped=*/nullptr)) {
       HRESULT hrWrite = HRESULT_FROM_WIN32(::GetLastError());
       LOGFN(ERROR) << "WriteFile hr=" << putHR(hrWrite);
     }

chrome/credential_provider/gaiacp/gaia_credential_base.h

@@ -44,10 +44,6 @@ class ATL_NO_VTABLE CGaiaCredentialBase
     : public IGaiaCredential,
       public ICredentialProviderCredential2 {
  public:
-  // Size in wchar_t of string buffer to pass account information to background
-  // process to save that information into the registry.
-  static const int kAccountInfoBufferSize = 2048;
-
   // Called when the DLL is registered or unregistered.
   static HRESULT OnDllRegisterServer();
   static HRESULT OnDllUnregisterServer();