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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

Attachment: fix v1.0

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Using sizeof isn't enough here... The docs say there will be a special error return if the buffer isn't big enough.

Comment 2

[(no longer active)]

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.

Comment 3

[(no longer active)]

I filed bug 634823 for the real fix, but I can own this one for 2.0 if we choose to block on it.

Comment 4

[(no longer active)]

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.

Comment 5

[(no longer active)]

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.

Comment 6

[Benjamin Smedberg]

I want whatever can be landed on branch. So I'd prefer to have a little code change as possible here, I think.

Comment 7

[(no longer active)]

I'll use nsIWindowsRegKey.

Comment 8

[(no longer active)]

Attachment: Patch (v1)

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.

Comment 9

[(no longer active)]

Attachment: Patch (v1.1)

(use spaces instead of tabs)

Comment 10

[(no longer active)]

Should this be a hard or soft blocker?

Comment 11

[Mike Shaver (:shaver emeritus)]

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.

Comment 12

[Jim Mathies [:jimm]]

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.

Comment 13

[Josh Aas]

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.

Comment 14

[(no longer active)]

(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.

Comment 15

[(no longer active)]

Jim, what do you think needs documenting here?

Comment 16

[(no longer active)]

(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...

Comment 17

[Jim Mathies [:jimm]]

(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.

Comment 18

[Jim Mathies [:jimm]]

(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

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/04527f9407f9

Comment 20

[(no longer active)]

(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.

Comment 21

[(no longer active)]

(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.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The landing of this patch has probably caused bug 635546.