Closed Bug 1440886 Opened 6 years ago Closed 6 years ago

Add a static analysis checker to disallow PR_LoadLibrary

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emk, Assigned: andi)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1440886 - Implement a static analysis checker to detect usage of PR_LoadLibrary and LoadLibraryA/LoadLibraryExA/LoadLibrary/LoadLibraryEx.
59 bytes, text/x-review-board-request
nika
: review+
Details
result_filtered
6.14 KB, text/plain
Details 
We have to use either
1. nsIFile::Load if nsIFile is available,
2. PR_LoadLibraryWithFlags with PR_LibSpec_PathnameU specified, or
3. Or operating system LoadLibrary(Ex)W directly if it is a platform-depedent code.

Otherwise Unicode file paths will break on Windows. See bug 1330529 about a live example.
We should also disallow:
1. PR_LibSpec_Pathname (but note that PR_LibSpec_PathnameU is Windows-specific),
2. LoadLibraryA and LoadLibraryExA, and
3. Unsuffixed LoadLibrary and LoadLibraryEx.
So let me see if i get this streight:

On Windows platform disallow: PR_LibSpec_Pathname
On All platforms disallow: LoadLibraryA, LoadLibraryExA, LoadLibrary, LoadLibraryEx.

If this is the case, the implementation is pretty straight forward, it can be done exclusively from the matcher perspective.
Flags: needinfo?(VYV03354)
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #2)
> So let me see if i get this streight:
> 
> On Windows platform disallow: PR_LibSpec_Pathname

and PR_LoadLibrary.

> On All platforms disallow: LoadLibraryA, LoadLibraryExA, LoadLibrary,
> LoadLibraryEx.

LoadLibraryA, LoadLibraryExA, LoadLibrary, LoadLibraryEx are all Windows-specific, but it would not hurt even if they are disabled on all platforms.
Flags: needinfo?(VYV03354)
See Also: → 1442275
Product: Core → Firefox Build System
Depends on: 1443438
I've dded this revision as a wip, it only needs tests that i will add after 1443438 lands. If you want to test please do on windows otherwise you will get a bounch of fp regarding PR_LoadLibrary.
If you wish to test it please apply first the patch from 1443438, since the current patch depends on that. The dependency will be resolved after 1443438 lands.
Flags: needinfo?(VYV03354)
Would be great to have a test for that!
Assignee: nobody → bpostelnicu
How to test this patch? No error happens with this patch (and the patch from 1443438) applied, but I don't think static analysis is running.
Flags: needinfo?(VYV03354)
Please see here: https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Clang_static_analysis
You’re interested in build phase static analysis.
If you have issues setting up the env let me know.
As 1443438 has been merged into m-c you no longer need the patch from the respective bug if you do a 'hg pull -u'. I will restrict thought this patch on windows since the current analysis if ran on anything different than windows will trigger a bunch of FP due to PR_LoadLibrary.
Right now i'm doing a build on windows with the clang-plugin and this patch activated to see how may occurrences we have in our code. I'm gonna post the result here.
First thing that triggered the checker was this definition:

https://dxr.mozilla.org/mozilla-central/source/js/src/vtune/ittnotify_config.h#279

>>#if ITT_PLATFORM==ITT_PLATFORM_WIN
>>#define __itt_get_proc(lib, name) GetProcAddress(lib, name)
>>#define __itt_mutex_init(mutex)   InitializeCriticalSection(mutex)
>>#define __itt_mutex_lock(mutex)   EnterCriticalSection(mutex)
>>#define __itt_mutex_unlock(mutex) LeaveCriticalSection(mutex)
>>#define __itt_load_lib(name)      LoadLibraryA(name)
>>#define __itt_unload_lib(handle)  FreeLibrary(handle)
>>#define __itt_system_error()      (int)GetLastError()
>>#define __itt_fstrcmp(s1, s2)     lstrcmpA(s1, s2)
>>#define __itt_fstrnlen(s, l)      strnlen_s(s, l)
>>#define __itt_fstrcpyn(s1, b, s2, l) strncpy_s(s1, b, s2, l)
>>#define __itt_fstrdup(s)          _strdup(s)
>>#define __itt_thread_id()         GetCurrentThreadId()
>>#define __itt_thread_yield()      SwitchToThread()

And it gets called from:

https://dxr.mozilla.org/mozilla-central/source/js/src/vtune/ittnotify_static.c#1125

>>                if (DL_SYMBOLS && (groups != __itt_group_none || lib_name != NULL))
>>                {
>>                    _N_(_ittapi_global).lib = __itt_load_lib((lib_name == NULL) ? ittnotify_lib_name : lib_name);
>>
>>                    if (_N_(_ittapi_global).lib != NULL)

I wonder why on windows is specified to be Ascii and not Wide char.

Sean can you please shed some light here?
Flags: needinfo?(sstangl)
Attached file result2 (obsolete) — 
These are the results of running the current static analysis on our tree. 
Can you please take a look and tell me what do you think. Are these potential issues legit and how should we move forward with the fixes?
Flags: needinfo?(VYV03354)
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #14)
> These are the results of running the current static analysis on our tree. 
> Can you please take a look and tell me what do you think. Are these
> potential issues legit and how should we move forward with the fixes?

I'm working on bug 1442275 to fix most issues, but I won't change third-party code and upstream the change. Is it possible to whitelist some directories?
Flags: needinfo?(VYV03354)
Flags: needinfo?(bpostelnicu)
Sure, but if we whitelist some directories other checker will no check into that directories. But where do you see issues detected by this checker in 3rd party code?
Flags: needinfo?(bpostelnicu)
Hm, I could not find third-party code in the result.

Is it possible to allow something like LoadLibraryA(<ASCII string literal>)?  It should be safe.
I though that we should blacklist LoadLibraryA, but we should permit that only when it calls with an ASCII String literal like:

LoadLibraryA("Some Path")?
Exactly.
(In reply to Masatoshi Kimura [:emk] from comment #19)
> Exactly.

Sure I'm gonna update the patch, do you want to do this only for LoadLibraryA? Do you still this be the case when you call LoadLibrary when UNICODE is not defined? Since in this case the underlying function that gets called it will also be LoadLibraryA
Same for all non-Unicode functions (PR_LoadLibrary, LoadLibraryA, LoadLibraryExA and LoadLibrary and LoadLibraryEx without UNICODE) defined.
(In reply to Masatoshi Kimura [:emk] from comment #21)
> Same for all non-Unicode functions (PR_LoadLibrary, LoadLibraryA,
> LoadLibraryExA and LoadLibrary and LoadLibraryEx without UNICODE) defined.

I've updated the patch with your suggestions. Do you manage to run the analysis or do you want me to run it?
I can not yet run the analysis locally. I'd appreciate if you run it.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #13)
> I wonder why on windows is specified to be Ascii and not Wide char.
> 
> Sean can you please shed some light here?

I really don't know. These files were provided by Intel, and we imported them without modification or review. They are used for allowing Firefox's JS JIT to be profiled by the Intel VTune Profiler.
Flags: needinfo?(sstangl)
Attached file result_filtered — 
With the latest version of the patch where we ignore functions that have the first argument as string literals, the number of issues was areduced a little bit.
Attachment #8957517 - Attachment is obsolete: true
If this patch suits our needs please let me know and I'll submit it for review.
Flags: needinfo?(VYV03354)
Looks good to me.
Flags: needinfo?(VYV03354)
Attachment #8956410 - Flags: review?(nika)
Comment on attachment 8956410 [details]
Bug 1440886 - Implement a static analysis checker to detect usage of PR_LoadLibrary and LoadLibraryA/LoadLibraryExA/LoadLibrary/LoadLibraryEx.

https://reviewboard.mozilla.org/r/225300/#review233298

::: build/clang-plugin/LoadLibraryUsageChecker.h:5
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef FopenUssageChecker_h__

Wrong include guard

::: build/clang-plugin/LoadLibraryUsageChecker.h:10
(Diff revision 4)
> +#ifndef FopenUssageChecker_h__
> +#define FopenUssageChecker_h__
> +
> +#include "plugin.h"
> +
> +class LoadLibraryUsageChecker : public BaseCheck {

Please add a comment to this file explaining why these functions are forbidden so that this fact is documented somewhere in-tree.

::: build/clang-plugin/LoadLibraryUsageChecker.h:18
(Diff revision 4)
> +      : BaseCheck(CheckName, Context) {}
> +  void registerMatchers(MatchFinder *AstMatcher) override;
> +  void check(const MatchFinder::MatchResult &Result) override;
> +};
> +
> +#endif

Can you add a // !defined(include_guard_h) here?

::: build/clang-plugin/tests/TestLoadLibraryUsage.cpp:3
(Diff revision 4)
> +#include <windows.h>
> +
> +void* PR_LoadLibrary(const char *name);

Let's just #include "prlink.h"

::: build/clang-plugin/tests/moz.build:52
(Diff revision 4)
>      'TestTrivialCtorDtor.cpp',
>  ]
>  
> +if CONFIG['OS_ARCH'] == 'WINNT':
> +    SOURCES += [
> +        'TestLoadLibraryUsage.cpp',

Should we have 2 copies of this test file, one where we #define UNICODE and one where we don't?
Attachment #8956410 - Flags: review?(nika) → review-
Comment on attachment 8956410 [details]
Bug 1440886 - Implement a static analysis checker to detect usage of PR_LoadLibrary and LoadLibraryA/LoadLibraryExA/LoadLibrary/LoadLibraryEx.

https://reviewboard.mozilla.org/r/225300/#review233332

::: build/clang-plugin/LoadLibraryUsageChecker.cpp:10
(Diff revision 5)
> +#include "LoadLibraryUsageChecker.h"
> +#include "CustomMatchers.h"
> +
> +// On MacOS the filesystem is UTF-8, on linux the canonical filename is 8-bit
> +// string. On Windows data loss conversion will occur. This checker restricts
> +// the use of ASCII file functions for loading libraries.

please add an "on windows"
Attachment #8956410 - Flags: review?(nika) → review+
Depends on: 1442275
How are we going to handle the usage of LoadLibraryExA and LoadLibraryA? This cannot land until all of the issues are fixed.
Flags: needinfo?(VYV03354)
Depends on: 1445601
Filed bug 1445601.
Flags: needinfo?(VYV03354)
Depends on: 1432653
I think now it is possible to land this. Please tell me if not.
I'm gonna test this again on the VM to see if there are still hidden issues, and if not I'm gonna push this to m-i. 
:emk thank you for the help!
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/827deadecf7e
Implement a static analysis checker to detect usage of PR_LoadLibrary and LoadLibraryA/LoadLibraryExA/LoadLibrary/LoadLibraryEx. r=Nika
Depends on: 1459902
https://hg.mozilla.org/mozilla-central/rev/827deadecf7e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: