Closed frank-dittrich closed 8 years ago
These are the scan-build error messages I get for master:
$ LC_ALL=C scan-build-3.5 make -s linux-x86-64-avx
scan-build: Using '/usr/lib/llvm-3.5/bin/clang' for static analysis
DES_std.c: In function 'DES_std_set_key':
DES_std.c:635:17: warning: array subscript is above array bounds [-Warray-bounds]
while (DES_key[i++]) k += 2;
^
AFS_fmt.c:371:31: warning: The right operand of '^' is a garbage value
DES_IV[0] = binary.data[0] ^ *ptr_binary++;
^ ~~~~~~~~~~~~~
1 warning generated.
idle.c:110:36: warning: Division by zero
calls_per_tick = calls_since_adj / (current - last_adj);
~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
memory.c:75:2: warning: Potential leak of memory pointed to by 'p'
return p;
^~~~~~~~
1 warning generated.
scan-build: 3 bugs found.
scan-build: Run 'scan-view /tmp/scan-build-2015-04-17-153544-26322-1' to examine bug reports.
This is (minus the colours for syntax highlighting etc.) what an individual report looks like (here the division by zero report):
void idle_yield(void)
87 {
88 #ifdef _POSIX_PRIORITY_SCHEDULING
89 static unsigned int calls_to_skip = 0;
90 static unsigned int calls_per_tick = 0;
91 static unsigned int calls_since_tick = 0;
92 static unsigned int calls_since_adj = 0;
93 static int calls_per_tick_known = 0;
94 static clock_t last_adj = 0;
95 clock_t last_check;
96 clock_t current;
97 int yield_calls;
98 struct tms buf;
99
100 if (!use_yield) return;
1
Assuming 'use_yield' is not equal to 0
→
2
←
Taking false branch
→
101
102 if (++calls_since_tick < calls_to_skip) return;
3
←
Taking false branch
→
103 calls_since_adj += calls_since_tick;
104 calls_since_tick = 0;
105
106 current = times(&buf);
107 if (!last_adj) last_adj = current;
4
←
Taking true branch
→
108
109 if (current - last_adj >= clk_tck) {
5
←
Taking true branch
→
110 calls_per_tick = calls_since_adj / (current - last_adj);
6
←
Division by zero
111 calls_since_adj = 0;
112 calls_per_tick_known = 2;
113 last_adj = current;
Some of the error messages are false positives.
475 if (db_entry->chains_buf == NULL)
2
←
Taking true branch
→
476 {
477 fprintf (stderr, "Out of memory trying to allocate %zu bytes\n", (size_t) chains_alloc_new * sizeof (chain_t));
478
479 #ifndef JTR_MODE
480 exit (-1);
481 #else
482 error();
483 #endif
484 }
485
486 memset (&db_entry->chains_buf[chains_alloc], 0, ALLOC_NEW_CHAINS * sizeof (chain_t));
3
←
Null pointer passed as an argument to a 'nonnull' parameter
487
scan-build didn't notice that the memset can never be reached with a NULL pointer.
Apparently a NEW bug from 2012: https://llvm.org/bugs/show_bug.cgi?id=12685 static-analyzer fails to see exit() in function w/ varargs resulting in a false positive
Adding __attribute__((__noreturn__))
to a few selected function definitions should help to avoid these false positives:
http://clang-analyzer.llvm.org/annotations.html#attr_noreturn
I'll test with the following patch:
diff --git a/src/misc.h b/src/misc.h
index e5653ee..e95fd5c 100644
--- a/src/misc.h
+++ b/src/misc.h
@@ -46,7 +46,13 @@
* Exit on error. Logs the event, closes john.pot and the log file, and
* terminates the process with non-zero exit status.
*/
-extern void real_error(char *file, int line);
+extern void real_error(char *file, int line)
+#ifdef __GNUC__
+ __attribute__((__noreturn__));
+#else
+ ;
+#endif
+ ;
#define error(...) real_error(__FILE__, __LINE__)
/*
@@ -54,6 +60,7 @@ extern void real_error(char *file, int line);
*/
extern void real_pexit(char *file, int line, char *format, ...)
#ifdef __GNUC__
+ __attribute__((__noreturn__))
__attribute__ ((format (printf, 3, 4)));
#else
;
commit 05754f018aa785a2f646861be38556ec7cd3ad65 got rid of 3 false positives, reducing the number of warnings from 122 to 119.
These are the 4 warnings that disappeared:
< dynamic_fmt.c:7908:10: warning: Dereference of null pointer
< valid = pFmtLocal->methods.valid(ciphertext, pFmtLocal);
< ^~~~~~~~~~~~~~~~~~~~~~~~
< pfx_fmt_plug.c:205:2: warning: Null pointer passed as an argument to a 'nonnull' parameter
< memcpy(&(psalt->pfx), p12, sizeof(PKCS12));
< ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
< pp.c:459:5: warning: Null pointer passed as an argument to a 'nonnull' parameter
< memset (&db_entry->elems_buf[elems_alloc], 0, ALLOC_NEW_ELEMS * sizeof (elem_t));
< ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
< pp.c:486:5: warning: Null pointer passed as an argument to a 'nonnull' parameter
< memset (&db_entry->chains_buf[chains_alloc], 0, ALLOC_NEW_CHAINS * sizeof (chain_t));
< ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
And this is the new warning:
> c3_fmt.c:126:9: warning: Value stored to 'salt' during its initialization is never read
> char *salt = tests[0].ciphertext;
> ^~~~ ~~~~~~~~~~~~~~~~~~~
The 2 bugs "Uninitialized argument value" are false positives.
But I was able to trigger an asan error for one of the 2 bugs "Out-of-bound array access":
git diff
diff --git a/run/john.conf b/run/john.conf
index 6b7e3ff..5002906 100644
--- a/run/john.conf
+++ b/run/john.conf
@@ -68,8 +68,8 @@ DefaultIncrementalLM = LM_ASCII
# %d/%m/%y %H:%M (day/mon/year hour:min)
# %m/%d/%y %H:%M (mon/day/year hour:min)
# %Y-%m-%d %H:%M (ISO 8601 style, 2011-05-06 18:10)
-TimeFormat = %Y-%m-%d %H:%M
-TimeFormat24 = %H:%M:%S
+TimeFormat = %Y-%m-%d %H:%M____________________________________________________________12345678901234567890123456789012345678901234567890123456789012345678901234567890
+TimeFormat24 = %H:%M:%S________________________________________________________________12345678901234567890123456789012345678901234567890123456789012345678901234567890
# For single mode, load the full GECOS field (before splitting) as one
# additional candidate. Normal behavior is to only load individual words
$ ./john hashes.sapb --wordlist
Loaded 10 password hashes with 10 different salts (sapb, SAP CODVN B (BCODE) [MD5 128/128 AVX 4x3])
Remaining 9 password hashes with 9 different salts
Will run 8 OpenMP threads
Press 'q' or Ctrl-C to abort, almost any other key for status
=================================================================
==11465== ERROR: AddressSanitizer: unknown-crash on address 0x000001384562 at pc 0x7fc74dad8645 bp 0x7ffd8fe13c20 sp 0x7ffd8fe133c8
WRITE of size 128 at 0x000001384562 thread T0
#0 0x7fc74dad8644 (/usr/lib64/libasan.so.0.0.0+0xf644)
#1 0x77e493 (/home/fd/git/JtR/run/john+0x77e493)
#2 0x77f748 (/home/fd/git/JtR/run/john+0x77f748)
#3 0x77fcea (/home/fd/git/JtR/run/john+0x77fcea)
#4 0x74ccaf (/home/fd/git/JtR/run/john+0x74ccaf)
#5 0x74d795 (/home/fd/git/JtR/run/john+0x74d795)
#6 0x342fa21d64 (/usr/lib64/libc-2.18.so+0x21d64)
#7 0x406534 (/home/fd/git/JtR/run/john+0x406534)
0x0000013845e0 is located 0 bytes to the right of global variable 's_ETA (status.c)' (0x1384560) of size 128
's_ETA (status.c)' is ascii string ' ('
Shadow bytes around the buggy address:
0x000080268850: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9
0x000080268860: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
0x000080268870: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9
0x000080268880: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9
0x000080268890: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9
=>0x0000802688a0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9[00]00 00 00
0x0000802688b0: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9
0x0000802688c0: 00 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
0x0000802688d0: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
0x0000802688e0: 04 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 04
0x0000802688f0: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 00 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Heap righ redzone: fb
Freed Heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
ASan internal: fe
==11465== ABORTING
$ ./asan_symbolize.py
#0 0x7fc74dad8644 (/usr/lib64/libasan.so.0.0.0+0xf644)
#1 0x77e493 (/home/fd/git/JtR/run/john+0x77e493)
#2 0x77f748 (/home/fd/git/JtR/run/john+0x77f748)
#3 0x77fcea (/home/fd/git/JtR/run/john+0x77fcea)
#4 0x74ccaf (/home/fd/git/JtR/run/john+0x74ccaf)
#5 0x74d795 (/home/fd/git/JtR/run/john+0x74d795)
#6 0x342fa21d64 (/usr/lib64/libc-2.18.so+0x21d64)
#7 0x406534 (/home/fd/git/JtR/run/john+0x406534)
llvm-symbolizer: for the -functions option: 'short' is invalid value for boolean argument! Try 0 or 1
#0 0x7fc74dad8644 in __interceptor_strncat _asan_rtl_
#1 0x77e493 in status_get_ETA /home/fd/git/JtR/src/status.c:270
#2 0x77f748 in status_print_cracking /home/fd/git/JtR/src/status.c:358
#3 0x77fcea in status_print /home/fd/git/JtR/src/status.c:482
#4 0x74ccaf in john_run /home/fd/git/JtR/src/john.c:1428
#5 0x74d795 in main /home/fd/git/JtR/src/john.c:1695
#6 0x342fa21d64 in ?? ??:0
#7 0x406534 in _start ??:?
From man strncat
:
The strncat() function is similar
(to strcat), except that
* it will use at most n bytes from src
Here, from src
means, it will happily write over the end of the destination buffer.
BTW: good luck finding this one with fuzzing.
Similar problems:
jumbo.c:123: strncat(ret, src, sizeof(ret) - 1);
opencl_pbkdf2_hmac_sha512_fmt_plug.c:301: strncat(out, &split_fields[1][i], sizeof(out) - 1);
status.c:268: strncat(s_ETA, " (", sizeof(s_ETA) - 1);
status.c:270: strncat(s_ETA, ETA, sizeof(s_ETA) - 1);
status.c:271: strncat(s_ETA, ")", sizeof(s_ETA) - 1);
status.c:293: strncat(s_ETA, " (ETA: ", sizeof(s_ETA) - 1);
status.c:298: strncat(s_ETA, ETA, sizeof(s_ETA) - 1);
status.c:299: strncat(s_ETA, ")", sizeof(s_ETA) - 1);
All these strncat errors are also reported in the "Anti-pattern in the argument" bug category. Here, the report also suggests a fix:
123 strncat(ret, src, sizeof(ret) - 1);
Potential buffer overflow. Replace with 'sizeof(ret) - strlen(ret) - 1' or use a safer 'strlcat' API
So we should replace all those strncat with strlcat and add a strlcat to jumbo.c for systems that doesn't have it.
Yes, I think that is the best way.
This issue is too old to be relevant. Any new run -> new issue(s).
On a 64bit Linux avx system:
Instead of calling
scan-view
, viewing file:///tmp/scan-build-2015-04-17-153339-21057-1/index.html in your preferred browser works as well.