Closed Bug 1344666 Opened 3 years ago Closed 3 years ago

Undefined behaviour in pr_inet_aton

Categories

(NSPR :: NSPR, defect)

defect
Not set

Tracking

(firefox-esr45 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 1 open bug)

Details

(Whiteboard: [adv-main55+])

Attachments

(1 file, 1 obsolete file)

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)
Attached patch inet_aton-fix.patch (obsolete) — Splinter Review
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)
(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.
(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
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 on attachment 8844793 [details] [diff] [review]
inet_aton-fix.patch

r=kaie

Can you check it in?
Attachment #8844793 - Flags: review?(kaie) → review+
https://hg.mozilla.org/projects/nspr/rev/b8ceb5cab1b2d6bc9dc188e9b2804fc6d6234bb1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.14
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.
Group: crypto-core-security → core-security-release
Target Milestone: 4.14 → 4.15
Whiteboard: [adv-main55+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.