Closed Bug 634819 Opened 9 years ago Closed 9 years ago

Registry calls in nsPluginDirServiceProvider.cpp do not use the correct buffer size, and do not have proper error handling

Categories

(Core :: Plug-ins, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- ?
status1.9.1 --- ?

People

(Reporter: jaas, Assigned: ehsan)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [sg:vector][hardblocker])

Attachments

(1 file, 2 obsolete files)

Attached patch fix v1.0 (obsolete) — Splinter Review
As pointed out by Ehsan here:

https://bugzilla.mozilla.org/show_bug.cgi?id=633463#c20

We should be passing a byte count, not a character count, to RegQueryValueExW and RegSetValueExW in the plugin dir service provider code.
Attachment #513028 - Flags: review?(bent.mozilla)
Using sizeof isn't enough here... The docs say there will be a special error return if the buffer isn't big enough.
Comment on attachment 513028 [details] [diff] [review]
fix v1.0

>diff --git a/modules/plugin/base/src/nsPluginDirServiceProvider.cpp b/modules/plugin/base/src/nsPluginDirServiceProvider.cpp
>--- a/modules/plugin/base/src/nsPluginDirServiceProvider.cpp
>+++ b/modules/plugin/base/src/nsPluginDirServiceProvider.cpp
>@@ -254,29 +254,29 @@ nsPluginDirServiceProvider::GetFile(cons
>     DWORD type;
>     WCHAR szKey[_MAX_PATH] = L"Software\\Netscape\\Netscape Navigator";

I'm not sure what this really does.  Does it put the string literal inside the buffer?  My C++ foo tells me that L"foo" is a string literal, and the only form of array initializer that I've seen (and know is correct) is something like:

WCHAR szKey[_MAX_PATH] = {L'\0'};

I'd sleep better at nights if you change all this kind of stuff to explicit wcscpy's.

>     WCHAR path[_MAX_PATH];
> 
>     result = ::RegOpenKeyExW(HKEY_LOCAL_MACHINE, szKey, 0, KEY_READ, &keyloc);
> 
>     if (result == ERROR_SUCCESS) {
>       WCHAR current_version[80];
>-      DWORD length = NS_ARRAY_LENGTH(current_version);
>+      DWORD length = sizeof(current_version);
> 
>       result = ::RegQueryValueExW(keyloc, L"CurrentVersion", NULL, &type,
>                                   (LPBYTE)&current_version, &length);
> 
>       ::RegCloseKey(keyloc);
>       wcscat(szKey, L"\\");
>       wcscat(szKey, current_version);
>       wcscat(szKey, L"\\Main");

There are three problems here.

1. We never check that the returned type is REG_SZ (and whether result is ERROR_SUCCESS for that matter), so we might just as well be dealing with a DWORD for example!  ;-)
2. REG_SZ values are not guaranteed to be null terminated, so we need to null-terminate them ourselves.
3. We don't check the returned length, which could be exactly 160, in which case the wcscat calls will happily overwrite the stack for us (if not because of point 2 above, that is!)

>       result = ::RegOpenKeyExW(HKEY_LOCAL_MACHINE, szKey, 0, KEY_READ, &keyloc);
> 
>       if (result == ERROR_SUCCESS) {
>-        DWORD pathlen = NS_ARRAY_LENGTH(path);
>+        DWORD pathlen = sizeof(path);
> 
>         result = ::RegQueryValueExW(keyloc, L"Plugins Directory", NULL, &type,
>                                    (LPBYTE)&path, &pathlen);
>         if (result == ERROR_SUCCESS) {
>           rv = NS_NewLocalFile(nsDependentString(path),
>                                PR_TRUE, getter_AddRefs(localFile));
>         }

Comments 1, 2, and 3 all apply here too.

>@@ -322,17 +322,17 @@ nsPluginDirServiceProvider::GetFile(cons
>                                             &numChars))
>       browserJavaVersion[0] = 0;
> 
>     // We must enumerate through the keys because what if there is
>     // more than one version?
>     do {
>       path[0] = 0;
>       numChars = _MAX_PATH;
>-      pathlen = NS_ARRAY_LENGTH(path);
>+      pathlen = sizeof(path);
>       result = ::RegEnumKeyExW(baseloc, index, curKey, &numChars, NULL, NULL,
>                                NULL, &modTime);

I'm not sure whether you left numChars as is on purpose or not, but this is correct indeed as it's the number of characters (not bytes).

>       index++;
> 
>       // Skip major.minor as it always points to latest in its family
>       numChars = 0;
>       for (WCHAR *p = curKey; *p; p++) {
>         if (*p == '.') {
>@@ -379,17 +379,17 @@ nsPluginDirServiceProvider::GetFile(cons
>       if (ERROR_SUCCESS == ::RegCreateKeyExW(HKEY_LOCAL_MACHINE, mozPath, 0,
>                                              NULL, REG_OPTION_NON_VOLATILE,
>                                              KEY_SET_VALUE|KEY_QUERY_VALUE,
>                                              NULL, &entryloc, NULL)) {
>         if (ERROR_SUCCESS != ::RegQueryValueExW(entryloc, L"CurrentVersion", 0,
>                                                NULL, NULL, NULL)) {
>           ::RegSetValueExW(entryloc, L"CurrentVersion", 0, REG_SZ,
>                            (const BYTE*) kMozillaVersion,
>-                           NS_ARRAY_LENGTH(kMozillaVersion));
>+                           sizeof(kMozillaVersion));

I'm really scared of this code:

static const WCHAR kMozillaVersion[] = NS_L(MOZILLA_VERSION);

I don't know what sizeof(kMozillaVersion) would be here...  It's best to be explicit, I'd say.

>         }
>         ::RegCloseKey(entryloc);
>       }
> 
>       wcscat(newestPath, L"\\bin");
> 
>       // See whether the "new_plugin" directory exists
>       WCHAR tmpPath[JAVA_PATH_SIZE];
>@@ -423,17 +423,17 @@ nsPluginDirServiceProvider::GetFile(cons
> 
>     // Look for the Quicktime system installation plugins directory
>     HKEY keyloc;
>     long result;
>     DWORD type;
>     verBlock qtVer;
>     ClearVersion(&qtVer);
>     WCHAR path[_MAX_PATH];
>-    DWORD pathlen = NS_ARRAY_LENGTH(path);
>+    DWORD pathlen = sizeof(path);
> 
>     // First we need to check the version of Quicktime via checking
>     // the EXE's version table
>     if (ERROR_SUCCESS == ::RegOpenKeyExW(HKEY_LOCAL_MACHINE,
>                                          L"software\\Microsoft\\Windows\\CurrentVersion\\App Paths\\QuickTimePlayer.exe",
>                                          0, KEY_READ, &keyloc)) {
>       if (ERROR_SUCCESS == ::RegQueryValueExW(keyloc, NULL, NULL, &type,
>                                               (LPBYTE)&path, &pathlen)) {

Same as above.

>@@ -442,17 +442,17 @@ nsPluginDirServiceProvider::GetFile(cons
>       ::RegCloseKey(keyloc);
>     }
>     if (CompareVersion(qtVer, minVer) < 0)
>       return rv;
> 
>     if (ERROR_SUCCESS == ::RegOpenKeyExW(HKEY_LOCAL_MACHINE,
>                                          L"software\\Apple Computer, Inc.\\QuickTime",
>                                         0, KEY_READ, &keyloc)) {
>-      DWORD pathlen = NS_ARRAY_LENGTH(path);
>+      DWORD pathlen = sizeof(path);
> 
>       result = ::RegQueryValueExW(keyloc, L"InstallDir", NULL, &type,
>                                   (LPBYTE)&path, &pathlen);
>       wcscat(path, L"\\Plugins");

Same as above.  Specifically, the wcscat call should happen after checking the result value.

>       if (result == ERROR_SUCCESS)
>         rv = NS_NewLocalFile(nsDependentString(path), PR_TRUE,
>                              getter_AddRefs(localFile));
>       ::RegCloseKey(keyloc);
>@@ -465,17 +465,17 @@ nsPluginDirServiceProvider::GetFile(cons
>     TranslateVersionStr(NS_ConvertASCIItoUTF16(strVer).get(), &minVer);
> 
>     // Look for Windows Media Player system installation plugins directory
>     HKEY keyloc;
>     DWORD type;
>     verBlock wmpVer;
>     ClearVersion(&wmpVer);
>     WCHAR path[_MAX_PATH];
>-    DWORD pathlen = NS_ARRAY_LENGTH(path);
>+    DWORD pathlen = sizeof(path);
> 
>     // First we need to check the version of WMP
>     if (ERROR_SUCCESS == ::RegOpenKeyExW(HKEY_LOCAL_MACHINE,
>                                          L"software\\Microsoft\\Windows\\CurrentVersion\\App Paths\\wmplayer.exe",
>                                          0, KEY_READ, &keyloc)) {
>       if (ERROR_SUCCESS == ::RegQueryValueExW(keyloc, NULL, NULL, &type,
>                                               (LPBYTE)&path, &pathlen)) {

Same as above, and ditto for the next two hunks.
Attachment #513028 - Flags: review?(bent.mozilla) → review-
Assignee: joshmoz → ehsan
blocking2.0: --- → ?
Attachment #513028 - Attachment is obsolete: true
I filed bug 634823 for the real fix, but I can own this one for 2.0 if we choose to block on it.
blocking2.0: ? → betaN+
Group: core-security
So, we read from HKEY_CURRENT_USER with this code, which can be written to using any program running under the user account.  While an exploit here requires local access, I'm really worried about this getting combined with another exploit (in another app maybe) to write bogus stuff to the stack.

A potential attacker can easily write whatever they want (read: binary data of their choice) on our stack here, which makes me really really worried about this bug.
Benjamin, do you want me to hack a quick version of the API as I dreamed about in bug 634823 for this bug (just enough to satisfy the API usage here)?

I hate to go over this again when we fix bug 634823.  I know the API very well, and I'm confident that fixing things the right way won't make my job in fixing this bug more difficult.
I want whatever can be landed on branch. So I'd prefer to have a little code change as possible here, I think.
Whiteboard: [sg:vector]
I'll use nsIWindowsRegKey.
Attached patch Patch (v1) (obsolete) — Splinter Review
This patch has two parts.  The first part modifies nsWindowsRegKey to correctly support REG_EXPAND_SZ values.  The second part basically rewrites nsPluginDirServiceProvider.cpp to make it use nsIWindowsRegKey.

I've written this very carefully, and have tested it with java, quicktime, flash, acrobat, and silverlight, and I've made sure that the list of plugins displayed in about:addons is the same with and without this patch.
Attachment #513319 - Flags: review?(joshmoz)
Attachment #513319 - Flags: review?(benjamin)
Attachment #513319 - Flags: approval2.0?
Summary: pass array length in bytes not characters for RegQueryValueExW and RegSetValueExW → Registry calls in nsPluginDirServiceProvider.cpp do not use the correct buffer size, and do not have proper error handling
Attached patch Patch (v1.1)Splinter Review
(use spaces instead of tabs)
Attachment #513319 - Attachment is obsolete: true
Attachment #513320 - Flags: review?(joshmoz)
Attachment #513320 - Flags: review?(benjamin)
Attachment #513320 - Flags: approval2.0?
Attachment #513319 - Flags: review?(joshmoz)
Attachment #513319 - Flags: review?(benjamin)
Attachment #513319 - Flags: approval2.0?
Should this be a hard or soft blocker?
Whiteboard: [sg:vector] → [sg:vector][has patch][needs review bsmedberg/josh]
Comment on attachment 513320 [details] [diff] [review]
Patch (v1.1)

Approved, mostly because we just added another use of this and it could bite us in ways that the longstanding bugs didn't.
Attachment #513320 - Flags: approval2.0? → approval2.0+
Whiteboard: [sg:vector][has patch][needs review bsmedberg/josh] → [sg:vector][has patch][needs review bsmedberg/josh][hardblocker]
Attachment #513320 - Flags: review?(benjamin) → review?(jmathies)
Comment on attachment 513320 [details] [diff] [review]
Patch (v1.1)

+    nsCOMPtr<nsIWindowsRegKey> regKey =
+      do_CreateInstance("@mozilla.org/windows-registry-key;1");
+    NS_ENSURE_TRUE(regKey, NS_ERROR_FAILURE);

Since we use this in every block of that big if statement lets save some code and just do it once at the top.

Generally the translation from the old code looks good.

+  // Expand the environment variables if needed
+  if (type == REG_EXPAND_SZ) {
+    const nsString &flatSource = PromiseFlatString(result);
+    resultLen = ExpandEnvironmentStringsW(flatSource.get(), NULL, 0);

I hope we don't rely on those non-escaped values anywhere, I'm guessing we don't. Let's file a follow up to clean up the few places where we do this after calling ReadStringValue.
Attachment #513320 - Flags: review?(jmathies) → review+
Comment on attachment 513320 [details] [diff] [review]
Patch (v1.1)

>     // Look for the Java OJI plugin via the JRE install path

You can remove this comment, we aren't looking for the old OJI plugin any more.

>-   static nsresult GetPLIDDirectoriesWithHKEY(HKEY aKey, 
>+   static nsresult GetPLIDDirectoriesWithRootKey(PRUint32 aKey,

Is this a safe assumption for 64-bit Windows? I don't know the Windows API here at all.
Attachment #513320 - Flags: review?(joshmoz) → review+
Keywords: dev-doc-needed
(In reply to comment #12)
> +  // Expand the environment variables if needed
> +  if (type == REG_EXPAND_SZ) {
> +    const nsString &flatSource = PromiseFlatString(result);
> +    resultLen = ExpandEnvironmentStringsW(flatSource.get(), NULL, 0);
> 
> I hope we don't rely on those non-escaped values anywhere, I'm guessing we
> don't. Let's file a follow up to clean up the few places where we do this after
> calling ReadStringValue.

Will do, but it's not a big deal, since expanding the variables twice will work fine.
Jim, what do you think needs documenting here?
(In reply to comment #13)
> >-   static nsresult GetPLIDDirectoriesWithHKEY(HKEY aKey, 
> >+   static nsresult GetPLIDDirectoriesWithRootKey(PRUint32 aKey,
> 
> Is this a safe assumption for 64-bit Windows? I don't know the Windows API here
> at all.

HKEY is pointer size, so we'll get 32-bit integers converted to 64-bit integers on Win64.  It's something which is worth a followup though...
(In reply to comment #15)
> Jim, what do you think needs documenting here?

The new behavior in ReadStringValue so people don't double expand it in the future.
(In reply to comment #17)
> (In reply to comment #15)
> > Jim, what do you think needs documenting here?
> 
> The new behavior in ReadStringValue so people don't double expand it in the
> future.

Specifically, we probably want to add a note here:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWindowsRegKey#readStringValue%28%29
http://hg.mozilla.org/mozilla-central/rev/04527f9407f9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:vector][has patch][needs review bsmedberg/josh][hardblocker] → [sg:vector][hardblocker]
Target Milestone: --- → mozilla2.0b12
(In reply to comment #12)
> +  // Expand the environment variables if needed
> +  if (type == REG_EXPAND_SZ) {
> +    const nsString &flatSource = PromiseFlatString(result);
> +    resultLen = ExpandEnvironmentStringsW(flatSource.get(), NULL, 0);
> 
> I hope we don't rely on those non-escaped values anywhere, I'm guessing we
> don't. Let's file a follow up to clean up the few places where we do this after
> calling ReadStringValue.

Filed bug 635368.
(In reply to comment #16)
> (In reply to comment #13)
> > >-   static nsresult GetPLIDDirectoriesWithHKEY(HKEY aKey, 
> > >+   static nsresult GetPLIDDirectoriesWithRootKey(PRUint32 aKey,
> > 
> > Is this a safe assumption for 64-bit Windows? I don't know the Windows API here
> > at all.
> 
> HKEY is pointer size, so we'll get 32-bit integers converted to 64-bit integers
> on Win64.  It's something which is worth a followup though...

Filed bug 635371.
status1.9.1: --- → ?
status1.9.2: --- → ?
The landing of this patch has probably caused bug 635546.
blocking2.0: betaN+ → ---
OS: Mac OS X → Windows 7
blocking2.0: --- → betaN+
Depends on: 648511
Group: core-security → core-security-release
Group: core-security-release
See Also: → 1566240
You need to log in before you can comment on or make changes to this bug.