Merge pull request #29 from sylveon/load-library-ex-enhancements

Enhance detection and diagnostics of LoadLibrary(Ex)
This commit is contained in:
David A. Wheeler 2021-01-11 19:15:20 -05:00 committed by GitHub
commit 29df9eb26e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 60 additions and 26 deletions

View File

@ -846,10 +846,27 @@ def cpp_unsafe_stl(hit):
if len(hit.parameters) <= 4:
add_warning(hit)
safe_load_library_flags = [
# Load only from the folder where the .exe file is located
'LOAD_LIBRARY_SEARCH_APPLICATION_DIR',
# Combination of application, System32 and user directories
'LOAD_LIBRARY_SEARCH_DEFAULT_DIRS',
# This flag requires an absolute path to the DLL to be passed
'LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR',
# Load only from System32
'LOAD_LIBRARY_SEARCH_SYSTEM32',
# Load only from directories specified with AddDllDirectory
# or SetDllDirectory
'LOAD_LIBRARY_SEARCH_USER_DIRS',
# Loading from the current directory will only proceed if
# the current directory is part of the safe load list
'LOAD_LIBRARY_SAFE_CURRENT_DIRS'
]
def load_library_ex(hit):
# If parameter 3 is 'LOAD_LIBRARY_SEARCH_SYSTEM32', it's safe.
# If parameter 3 has one of the flags below, it's safe.
if (len(hit.parameters) >= 4 and
hit.parameters[3] == 'LOAD_LIBRARY_SEARCH_SYSTEM32'):
any(flag in hit.parameters[3] for flag in safe_load_library_flags)):
return
normal(hit)
@ -1298,12 +1315,12 @@ c_ruleset = {
"LoadLibrary":
(normal, 3, "Ensure that the full path to the library is specified, or current directory may be used (CWE-829, CWE-20)",
"Use registry entry or GetWindowsDirectory to find library path, if you aren't already",
"Use LoadLibraryEx with one of the search flags, or call SetSearchPathMode to use a safe search path, or pass a full path to the library",
"misc", "", {'input': 1}),
"LoadLibraryEx":
(load_library_ex, 3, "Ensure that the full path to the library is specified, or current directory may be used (CWE-829, CWE-20)",
"Use registry entry or GetWindowsDirectory to find library path, if you aren't already",
"Use a flag like LOAD_LIBRARY_SEARCH_SYSTEM32 or LOAD_LIBRARY_SEARCH_APPLICATION_DIR to search only desired folders",
"misc", "", {'input': 1}),
"SetSecurityDescriptorDacl":

View File

@ -5,8 +5,8 @@ Number of rules (primarily dangerous function names) in C/C++ ruleset: 222
ANALYSIS SUMMARY:
No hits found.
Lines analyzed = 123
Physical Source Lines of Code (SLOC) = 85
Lines analyzed = 125
Physical Source Lines of Code (SLOC) = 86
Hits@level = [0] 0 [1] 0 [2] 0 [3] 0 [4] 0 [5] 0
Hits@level+ = [0+] 0 [1+] 0 [2+] 0 [3+] 0 [4+] 0 [5+] 0
Hits/KSLOC@level+ = [0+] 0 [1+] 0 [2+] 0 [3+] 0 [4+] 0 [5+] 0

View File

@ -18,7 +18,8 @@ test.c,49,3,4,buffer,_mbscpy,Does not check for buffer overflows when copying to
test.c,56,3,4,buffer,lstrcat,Does not check for buffer overflows when concatenating to destination [MS-banned] (CWE-120),,,CWE-120," lstrcat(d,s);",364b4c512862fdccbca27d2fa7737995b5d24b637a760976c940ae636218d340
test.c,79,3,3,shell,CreateProcess,This causes a new process to execute and is difficult to use safely (CWE-78),"Specify the application path in the first argument, NOT as part of the second, or embedded spaces could allow an attacker to force a different program to run",,CWE-78," CreateProcess(NULL, ""C:\\Program Files\\GoodGuy\\GoodGuy.exe -x"", """");",3c712b38d0857bde3832d85ad35ac9859be55c5f5f1c20af659a577dd4d0acbf
test.c,79,3,3,shell,CreateProcess,This causes a new process to execute and is difficult to use safely (CWE-78),"Specify the application path in the first argument, NOT as part of the second, or embedded spaces could allow an attacker to force a different program to run",,CWE-78," CreateProcess(NULL, ""C:\\Program Files\\GoodGuy\\GoodGuy.exe -x"", """");",3c712b38d0857bde3832d85ad35ac9859be55c5f5f1c20af659a577dd4d0acbf
test.c,97,20,3,buffer,getopt_long,"Some older implementations do not protect against internal buffer overflows (CWE-120, CWE-20)","Check implementation on installation, or limit the size of all string inputs",,"CWE-120, CWE-20"," while ((optc = getopt_long (argc, argv, ""a"",longopts, NULL )) != EOF) {",5bedf6e5bccf596008ef191ec4c5d4cc51a32cff0c05ef62d5f10fab93d0cc24
test.c,81,10,3,misc,LoadLibraryEx,"Ensure that the full path to the library is specified, or current directory may be used (CWE-829, CWE-20)",Use a flag like LOAD_LIBRARY_SEARCH_SYSTEM32 or LOAD_LIBRARY_SEARCH_APPLICATION_DIR to search only desired folders,,"CWE-829, CWE-20"," (void) LoadLibraryEx(L""user32.dll"", nullptr, LOAD_LIBRARY_AS_DATAFILE);",b1f99ecaa31e682487d795afbf03282fd56ad9f2aa630d0196219b277d2a68c9
test.c,99,20,3,buffer,getopt_long,"Some older implementations do not protect against internal buffer overflows (CWE-120, CWE-20)","Check implementation on installation, or limit the size of all string inputs",,"CWE-120, CWE-20"," while ((optc = getopt_long (argc, argv, ""a"",longopts, NULL )) != EOF) {",5bedf6e5bccf596008ef191ec4c5d4cc51a32cff0c05ef62d5f10fab93d0cc24
test.c,16,2,2,buffer,strcpy,Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120),"Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused)",Risk is low because the source is a constant string.,CWE-120," strcpy(a, gettext(""Hello there"")); // Did this work?",d64070fb93ff0bb797fb926f4dddc7212d42f77e288d5ceb0cd30ed2979fa28d
test.c,19,2,2,buffer,sprintf,Does not check for buffer overflows (CWE-120),"Use sprintf_s, snprintf, or vsnprintf",Risk is low because the source has a constant maximum length.,CWE-120," sprintf(s, ""hello"");",907b46be1c3ea7b38f90a4d1b0f43b7751cd8cbe38fae840930ff006b702157d
test.c,45,3,2,buffer,char,"Statically-sized arrays can be improperly restricted, leading to potential overflows or other issues (CWE-119!/CWE-120)","Perform bounds checking, use functions that limit length, or ensure that the size is larger than the maximum possible length",,CWE-119!/CWE-120, char d[20];,36c87517700337a59cc3ad3218cfdde56cad37d69cdeccee5a55ab232d5c7946
@ -27,7 +28,7 @@ test.c,50,3,2,buffer,memcpy,Does not check for buffer overflows when copying to
test.c,53,3,2,buffer,memcpy,Does not check for buffer overflows when copying to destination (CWE-120),Make sure destination can always hold the source data,,CWE-120," memcpy(&n,s,sizeof(s)); // fail - sizeof not of destination",01bcc2c8ba2d928ac3315b4dcc6593042ea05e62888a10a6d2cf16797a65ed32
test.c,54,3,2,buffer,memcpy,Does not check for buffer overflows when copying to destination (CWE-120),Make sure destination can always hold the source data,,CWE-120," memcpy(d,s,n); // fail - size unguessable",2517a2fb5981193a6017cca660d16e85aab133706cbec302df97aaa623fc77ef
test.c,55,3,2,buffer,CopyMemory,Does not check for buffer overflows when copying to destination (CWE-120),Make sure destination can always hold the source data,,CWE-120," CopyMemory(d,s);",977f8c805ddd76ff32e0f7aea08701ba97d9ce6955136e98b308ed4f70eb2e11
test.c,103,7,2,misc,fopen,"Check when opening files - can an attacker redirect it (via symlinks), force the opening of special file type (e.g., device files), move things around to create a race condition, control its ancestors, or change its contents? (CWE-362)",,,CWE-362," f = fopen(""/etc/passwd"", ""r""); ",2ec6928c77a8b54caa61d0459f367c4394ee1f5e6f488753f587bfa9c780bad8
test.c,105,7,2,misc,fopen,"Check when opening files - can an attacker redirect it (via symlinks), force the opening of special file type (e.g., device files), move things around to create a race condition, control its ancestors, or change its contents? (CWE-362)",,,CWE-362," f = fopen(""/etc/passwd"", ""r""); ",2ec6928c77a8b54caa61d0459f367c4394ee1f5e6f488753f587bfa9c780bad8
test.c,15,2,1,buffer,strcpy,Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120),"Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused)",Risk is low because the source is a constant character.,CWE-120," strcpy(a, ""\n""); // Did this work?",0badc5f4c500d17b42794feaca54ee0f49e607a32510af3ed749579001017edb
test.c,18,2,1,buffer,sprintf,Does not check for buffer overflows (CWE-120),"Use sprintf_s, snprintf, or vsnprintf",Risk is low because the source is a constant character.,CWE-120," sprintf(s, ""\n"");",c65fbd60851f3c8ace22332805966606488c0d242c1823493c582e267609b1a7
test.c,26,2,1,buffer,scanf,It's unclear if the %s limit in the format string is small enough (CWE-120),"Check that the limit is sufficiently small, or use a different input function",,CWE-120," scanf(""%10s"", s);",e24c4c801f10acfa93098b2bef58524efe4f88237f2dd8b58be9afa838616afe

1 File Line Column Level Category Name Warning Suggestion Note CWEs Context Fingerprint
18 test.c 56 3 4 buffer lstrcat Does not check for buffer overflows when concatenating to destination [MS-banned] (CWE-120) CWE-120 lstrcat(d,s); 364b4c512862fdccbca27d2fa7737995b5d24b637a760976c940ae636218d340
19 test.c 79 3 3 shell CreateProcess This causes a new process to execute and is difficult to use safely (CWE-78) Specify the application path in the first argument, NOT as part of the second, or embedded spaces could allow an attacker to force a different program to run CWE-78 CreateProcess(NULL, "C:\\Program Files\\GoodGuy\\GoodGuy.exe -x", ""); 3c712b38d0857bde3832d85ad35ac9859be55c5f5f1c20af659a577dd4d0acbf
20 test.c 79 3 3 shell CreateProcess This causes a new process to execute and is difficult to use safely (CWE-78) Specify the application path in the first argument, NOT as part of the second, or embedded spaces could allow an attacker to force a different program to run CWE-78 CreateProcess(NULL, "C:\\Program Files\\GoodGuy\\GoodGuy.exe -x", ""); 3c712b38d0857bde3832d85ad35ac9859be55c5f5f1c20af659a577dd4d0acbf
21 test.c 97 81 20 10 3 buffer misc getopt_long LoadLibraryEx Some older implementations do not protect against internal buffer overflows (CWE-120, CWE-20) Ensure that the full path to the library is specified, or current directory may be used (CWE-829, CWE-20) Check implementation on installation, or limit the size of all string inputs Use a flag like LOAD_LIBRARY_SEARCH_SYSTEM32 or LOAD_LIBRARY_SEARCH_APPLICATION_DIR to search only desired folders CWE-120, CWE-20 CWE-829, CWE-20 while ((optc = getopt_long (argc, argv, "a",longopts, NULL )) != EOF) { (void) LoadLibraryEx(L"user32.dll", nullptr, LOAD_LIBRARY_AS_DATAFILE); 5bedf6e5bccf596008ef191ec4c5d4cc51a32cff0c05ef62d5f10fab93d0cc24 b1f99ecaa31e682487d795afbf03282fd56ad9f2aa630d0196219b277d2a68c9
22 test.c 99 20 3 buffer getopt_long Some older implementations do not protect against internal buffer overflows (CWE-120, CWE-20) Check implementation on installation, or limit the size of all string inputs CWE-120, CWE-20 while ((optc = getopt_long (argc, argv, "a",longopts, NULL )) != EOF) { 5bedf6e5bccf596008ef191ec4c5d4cc51a32cff0c05ef62d5f10fab93d0cc24
23 test.c 16 2 2 buffer strcpy Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120) Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused) Risk is low because the source is a constant string. CWE-120 strcpy(a, gettext("Hello there")); // Did this work? d64070fb93ff0bb797fb926f4dddc7212d42f77e288d5ceb0cd30ed2979fa28d
24 test.c 19 2 2 buffer sprintf Does not check for buffer overflows (CWE-120) Use sprintf_s, snprintf, or vsnprintf Risk is low because the source has a constant maximum length. CWE-120 sprintf(s, "hello"); 907b46be1c3ea7b38f90a4d1b0f43b7751cd8cbe38fae840930ff006b702157d
25 test.c 45 3 2 buffer char Statically-sized arrays can be improperly restricted, leading to potential overflows or other issues (CWE-119!/CWE-120) Perform bounds checking, use functions that limit length, or ensure that the size is larger than the maximum possible length CWE-119!/CWE-120 char d[20]; 36c87517700337a59cc3ad3218cfdde56cad37d69cdeccee5a55ab232d5c7946
28 test.c 53 3 2 buffer memcpy Does not check for buffer overflows when copying to destination (CWE-120) Make sure destination can always hold the source data CWE-120 memcpy(&n,s,sizeof(s)); // fail - sizeof not of destination 01bcc2c8ba2d928ac3315b4dcc6593042ea05e62888a10a6d2cf16797a65ed32
29 test.c 54 3 2 buffer memcpy Does not check for buffer overflows when copying to destination (CWE-120) Make sure destination can always hold the source data CWE-120 memcpy(d,s,n); // fail - size unguessable 2517a2fb5981193a6017cca660d16e85aab133706cbec302df97aaa623fc77ef
30 test.c 55 3 2 buffer CopyMemory Does not check for buffer overflows when copying to destination (CWE-120) Make sure destination can always hold the source data CWE-120 CopyMemory(d,s); 977f8c805ddd76ff32e0f7aea08701ba97d9ce6955136e98b308ed4f70eb2e11
31 test.c 103 105 7 2 misc fopen Check when opening files - can an attacker redirect it (via symlinks), force the opening of special file type (e.g., device files), move things around to create a race condition, control its ancestors, or change its contents? (CWE-362) CWE-362 f = fopen("/etc/passwd", "r"); 2ec6928c77a8b54caa61d0459f367c4394ee1f5e6f488753f587bfa9c780bad8
32 test.c 15 2 1 buffer strcpy Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120) Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused) Risk is low because the source is a constant character. CWE-120 strcpy(a, "\n"); // Did this work? 0badc5f4c500d17b42794feaca54ee0f49e607a32510af3ed749579001017edb
33 test.c 18 2 1 buffer sprintf Does not check for buffer overflows (CWE-120) Use sprintf_s, snprintf, or vsnprintf Risk is low because the source is a constant character. CWE-120 sprintf(s, "\n"); c65fbd60851f3c8ace22332805966606488c0d242c1823493c582e267609b1a7
34 test.c 26 2 1 buffer scanf It's unclear if the %s limit in the format string is small enough (CWE-120) Check that the limit is sufficiently small, or use a different input function CWE-120 scanf("%10s", s); e24c4c801f10acfa93098b2bef58524efe4f88237f2dd8b58be9afa838616afe

View File

@ -171,7 +171,17 @@ Examining test2.c <br>
<pre>
CreateProcess(NULL, "C:\\Program Files\\GoodGuy\\GoodGuy.exe -x", "");
</pre>
<li>test.c:97: <b> [3] </b> (buffer) <i> getopt_long:
<li>test.c:81: <b> [3] </b> (misc) <i> LoadLibraryEx:
Ensure that the full path to the library is specified, or current directory
may be used (<a
href="https://cwe.mitre.org/data/definitions/829.html">CWE-829</a>, <a
href="https://cwe.mitre.org/data/definitions/20.html">CWE-20</a>). Use a
flag like LOAD_LIBRARY_SEARCH_SYSTEM32 or
LOAD_LIBRARY_SEARCH_APPLICATION_DIR to search only desired folders. </i>
<pre>
(void) LoadLibraryEx(L"user32.dll", nullptr, LOAD_LIBRARY_AS_DATAFILE);
</pre>
<li>test.c:99: <b> [3] </b> (buffer) <i> getopt_long:
Some older implementations do not protect against internal buffer overflows
(<a href="https://cwe.mitre.org/data/definitions/120.html">CWE-120</a>, <a
href="https://cwe.mitre.org/data/definitions/20.html">CWE-20</a>). Check
@ -243,7 +253,7 @@ Examining test2.c <br>
<pre>
CopyMemory(d,s);
</pre>
<li>test.c:103: <b> [2] </b> (misc) <i> fopen:
<li>test.c:105: <b> [2] </b> (misc) <i> fopen:
Check when opening files - can an attacker redirect it (via symlinks),
force the opening of special file type (e.g., device files), move things
around to create a race condition, control its ancestors, or change its
@ -321,15 +331,15 @@ Examining test2.c <br>
</ul>
<h2>Analysis Summary</h2>
<p>
Hits = 38
Hits = 39
<br>
Lines analyzed = 124
Lines analyzed = 126
<br>
Physical Source Lines of Code (SLOC) = 85
Physical Source Lines of Code (SLOC) = 86
<br>
Hits@level = [0] 16 [1] 9 [2] 9 [3] 3 [4] 10 [5] 7 <br>
Hits@level+ = [0+] 54 [1+] 38 [2+] 29 [3+] 20 [4+] 17 [5+] 7 <br>
Hits/KSLOC@level+ = [0+] 635.294 [1+] 447.059 [2+] 341.176 [3+] 235.294 [4+] 200 [5+] 82.3529 <br>
Hits@level = [0] 16 [1] 9 [2] 9 [3] 4 [4] 10 [5] 7 <br>
Hits@level+ = [0+] 55 [1+] 39 [2+] 30 [3+] 21 [4+] 17 [5+] 7 <br>
Hits/KSLOC@level+ = [0+] 639.535 [1+] 453.488 [2+] 348.837 [3+] 244.186 [4+] 197.674 [5+] 81.3953 <br>
Suppressed hits = 2 (use --neverignore to show them)
<br>
Minimum risk level = 1

View File

@ -75,7 +75,11 @@ test.c:79: [3] (shell) CreateProcess:
(CWE-78). Specify the application path in the first argument, NOT as part
of the second, or embedded spaces could allow an attacker to force a
different program to run.
test.c:97: [3] (buffer) getopt_long:
test.c:81: [3] (misc) LoadLibraryEx:
Ensure that the full path to the library is specified, or current directory
may be used (CWE-829, CWE-20). Use a flag like LOAD_LIBRARY_SEARCH_SYSTEM32
or LOAD_LIBRARY_SEARCH_APPLICATION_DIR to search only desired folders.
test.c:99: [3] (buffer) getopt_long:
Some older implementations do not protect against internal buffer overflows
(CWE-120, CWE-20). Check implementation on installation, or limit the size
of all string inputs.
@ -108,7 +112,7 @@ test.c:54: [2] (buffer) memcpy:
test.c:55: [2] (buffer) CopyMemory:
Does not check for buffer overflows when copying to destination (CWE-120).
Make sure destination can always hold the source data.
test.c:103: [2] (misc) fopen:
test.c:105: [2] (misc) fopen:
Check when opening files - can an attacker redirect it (via symlinks),
force the opening of special file type (e.g., device files), move things
around to create a race condition, control its ancestors, or change its
@ -146,12 +150,12 @@ test.c:70: [1] (buffer) MultiByteToWideChar:
ANALYSIS SUMMARY:
Hits = 38
Lines analyzed = 124
Physical Source Lines of Code (SLOC) = 85
Hits@level = [0] 16 [1] 9 [2] 9 [3] 3 [4] 10 [5] 7
Hits@level+ = [0+] 54 [1+] 38 [2+] 29 [3+] 20 [4+] 17 [5+] 7
Hits/KSLOC@level+ = [0+] 635.294 [1+] 447.059 [2+] 341.176 [3+] 235.294 [4+] 200 [5+] 82.3529
Hits = 39
Lines analyzed = 126
Physical Source Lines of Code (SLOC) = 86
Hits@level = [0] 16 [1] 9 [2] 9 [3] 4 [4] 10 [5] 7
Hits@level+ = [0+] 55 [1+] 39 [2+] 30 [3+] 21 [4+] 17 [5+] 7
Hits/KSLOC@level+ = [0+] 639.535 [1+] 453.488 [2+] 348.837 [3+] 244.186 [4+] 197.674 [5+] 81.3953
Suppressed hits = 2 (use --neverignore to show them)
Minimum risk level = 1

View File

@ -77,8 +77,10 @@ demo2() {
SetSecurityDescriptorDacl(&sd,TRUE,NULL,FALSE);
/* This one is a bad idea - first param shouldn't be NULL */
CreateProcess(NULL, "C:\\Program Files\\GoodGuy\\GoodGuy.exe -x", "");
/* This should be ignored */
(void) LoadLibraryEx(L"user32.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32);
/* Bad, may load from current directory */
(void) LoadLibraryEx(L"user32.dll", nullptr, LOAD_LIBRARY_AS_DATAFILE);
/* This should be ignored, since it's loading only from System32 */
(void) LoadLibraryEx(L"user32.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32 | LOAD_LIBRARY_REQUIRE_SIGNED_TARGET);
/* Test interaction of quote characters */
printf("%c\n", 'x');
printf("%c\n", '"');