Closed
Bug 1344666
Opened 7 years ago
Closed 7 years ago
Undefined behaviour in pr_inet_aton
Categories
(NSPR :: NSPR, defect)
NSPR
NSPR
Tracking
(firefox-esr45 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)
RESOLVED
FIXED
4.15
People
(Reporter: franziskus, Assigned: franziskus)
References
(Blocks 1 open bug)
Details
(Whiteboard: [adv-main55+])
Attachments
(1 file, 1 obsolete file)
1.42 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
pr_inet_aton makes funny shifts overflowing some integers. > PRUint8 parts[4]; > val |= parts[0] << 24; > val |= (parts[0] << 24) | (parts[1] << 16) | (parts[1] << 8) > INFO: Seed: 3632612024 > INFO: Loaded 0 modules (0 guards): > ../dist/Debug/bin/nssfuzz-certs: Running 1 inputs 1 time(s) each. > Running: crash-c92a7a697a60de74d6bd27ceb35f2a785be8cd6d > 15121490.00000.0000000000.00 > ../../../../pr/src/misc/praton.c:192:26: runtime error: left shift of 128 by 24 places cannot be represented in type 'int' > #0 0xd45116 in pr_inet_aton /home/franziskus/Code/fuzzing/local-fuzzer/nspr/Debug/pr/src/misc/../../../../pr/src/misc/praton.c:192:26 > #1 0xd3fab1 in pr_StringToNetAddrFB /home/franziskus/Code/fuzzing/local-fuzzer/nspr/Debug/pr/src/misc/../../../../pr/src/misc/prnetdb.c:2182:10 > #2 0xd3f99c in PR_StringToNetAddr /home/franziskus/Code/fuzzing/local-fuzzer/nspr/Debug/pr/src/misc/../../../../pr/src/misc/prnetdb.c:2225:16 > #3 0x576cbf in cert_IsIPAddr /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../lib/certdb/certdb.c:1281:31 > #4 0x576a13 in CERT_VerifyCertName /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../lib/certdb/certdb.c:1759:27 > #5 0x517410 in LLVMFuzzerTestOneInput /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../fuzz/certs_target.cc:65:11 > #6 0x51f7d4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../fuzz/libFuzzer/FuzzerLoop.cpp:553:13 > #7 0x520149 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long) /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../fuzz/libFuzzer/FuzzerLoop.cpp:504:3 > #8 0x54ea07 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../fuzz/libFuzzer/FuzzerDriver.cpp:268:6 > #9 0x55430c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../fuzz/libFuzzer/FuzzerDriver.cpp:517:9 > #10 0x52f352 in main /home/franziskus/Code/fuzzing/local-fuzzer/nss/out/Debug/../../fuzz/libFuzzer/FuzzerMain.cpp:20:10 > #11 0x7f1c9f82a290 in __libc_start_main (/usr/lib/libc.so.6+0x20290) > #12 0x42ac29 in _start (/home/franziskus/Code/fuzzing/local-fuzzer/dist/Debug/bin/nssfuzz-certs+0x42ac29)
Assignee | ||
Comment 1•7 years ago
|
||
There are two things going on here. 1) PRUint8 (unsigned char) doesn't seem to be considered unsigned (but int). That's why UBSAN is sad. We can silence it by casting to unsigned int before shifting. 2) pr_inet_aton accepts IPs as valid that shouldn't be. In particular IPs containing line breaks (LF or CR) are considered valid. This shouldn't be. I'm not entirely sure about the security impact of this. I'd say sec-moderate because an attacker can supply invalid IP addresses that Firefox and NSS will accept and process further.
Attachment #8843998 -
Flags: review?(ted)
Attachment #8843998 -
Flags: review?(kaie)
Comment 2•7 years ago
|
||
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #1) > 2) pr_inet_aton accepts IPs as valid that shouldn't be. In particular IPs > containing line breaks (LF or CR) are considered valid. This shouldn't be. This conclusion doesn't seem right. The check for _isspace is outside of the loop. The whitespace is allowed only after the IP, not inside the IP. If there isn't an issue here, we should keep that code.
Comment 3•7 years ago
|
||
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #1) > 1) PRUint8 (unsigned char) doesn't seem to be considered unsigned (but int). > That's why UBSAN is sad. We can silence it by casting to unsigned int before > shifting. I agree with your fix. However, I don't think "unsigned char" is considered "signed", there's probably a different explanation. The compiler might know that "unsigned char" is too small to hold the result, and might attempt a conversion to a default type, which might be int. But int is still too small, and therefore it's warning that a human should make a decision. r=kaie, for the casts, not for _isspace
Assignee | ||
Comment 4•7 years ago
|
||
As discussed, let's only do the casts.
Attachment #8843998 -
Attachment is obsolete: true
Attachment #8843998 -
Flags: review?(ted)
Attachment #8843998 -
Flags: review?(kaie)
Attachment #8844793 -
Flags: review?(kaie)
Comment 5•7 years ago
|
||
Comment on attachment 8844793 [details] [diff] [review] inet_aton-fix.patch r=kaie Can you check it in?
Attachment #8844793 -
Flags: review?(kaie) → review+
Assignee | ||
Comment 6•7 years ago
|
||
https://hg.mozilla.org/projects/nspr/rev/b8ceb5cab1b2d6bc9dc188e9b2804fc6d6234bb1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.14
Comment 7•7 years ago
|
||
Talked to Franziskus on IRC about this and he said it's not worth backporting to older releases. Fx55 will pick up this fix via an updated NSPR release which is coming soon.
status-firefox53:
--- → wontfix
status-firefox54:
--- → wontfix
status-firefox55:
--- → fixed
status-firefox-esr45:
--- → wontfix
status-firefox-esr52:
--- → wontfix
Updated•7 years ago
|
Group: crypto-core-security → core-security-release
Updated•7 years ago
|
Target Milestone: 4.14 → 4.15
Updated•7 years ago
|
Whiteboard: [adv-main55+]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•