User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

While fuzzing icalparser_parse_string from Thunderbird's fork of libical built with ASAN, a heap buffer overflow read triggers in parser_get_next_char with some inputs. A reliable reproducer is attached.

Actual results:

==2528==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700000040f at pc 0x00000052c655 bp 0x7ffef533cc30 sp 0x7ffef533cc28
READ of size 1 at 0x60700000040f thread T0
#0 0x52c654 in parser_get_next_char /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:208:38
#1 0x5183bc in parser_get_prop_name /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:247:9
#2 0x5183bc in icalparser_add_line /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:700
#3 0x517e1b in icalparser_parse /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:623:11
#4 0x4fd243 in icalparser_parse_string /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:1236:9
#5 0x4fa975 in LLVMFuzzerTestOneInput (/opt/libfuzzer/thunderbird_libical_fuzzer+0x4fa975)
#6 0x43a681 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/opt/libfuzzer/thunderbird_libical_fuzzer+0x43a681)
#7 0x424327 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/opt/libfuzzer/thunderbird_libical_fuzzer+0x424327)
#8 0x42a4c1 in fuzzer::FuzzerDriver(int*, char***, int ()(unsigned char const, unsigned long)) (/opt/libfuzzer/thunderbird_libical_fuzzer+0x42a4c1)
#9 0x453f62 in main (/opt/libfuzzer/thunderbird_libical_fuzzer+0x453f62)
#10 0x7f28bc086b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
#11 0x41dc49 in _start (/opt/libfuzzer/thunderbird_libical_fuzzer+0x41dc49)

0x60700000040f is located 1 bytes to the left of 80-byte region [0x607000000410,0x607000000460)
allocated by thread T0 here:
#0 0x4cbb5d in malloc (/opt/libfuzzer/thunderbird_libical_fuzzer+0x4cbb5d)
#1 0x4fe7bd in icalmemory_new_buffer /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalmemory.c:266:15
#2 0x51719b in icalparser_get_line /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:453:21
#3 0x517e0d in icalparser_parse /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:621:13
#4 0x4fd243 in icalparser_parse_string /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:1236:9
#5 0x4fa975 in LLVMFuzzerTestOneInput (/opt/libfuzzer/thunderbird_libical_fuzzer+0x4fa975)
#6 0x43a681 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/opt/libfuzzer/thunderbird_libical_fuzzer+0x43a681)
#7 0x424327 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/opt/libfuzzer/thunderbird_libical_fuzzer+0x424327)
#8 0x42a4c1 in fuzzer::FuzzerDriver(int*, char***, int ()(unsigned char const, unsigned long)) (/opt/libfuzzer/thunderbird_libical_fuzzer+0x42a4c1)
#9 0x453f62 in main (/opt/libfuzzer/thunderbird_libical_fuzzer+0x453f62)
#10 0x7f28bc086b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

SUMMARY: AddressSanitizer: heap-buffer-overflow /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:208:38 in parser_get_next_char

Expected results:

No heap buffer overflow.

When the vulnerable function in Thunderbird is replaced with current implementation in libical upstream (https://github.com/libical/libical/blob/master/src/libical/icalparser.c#L147) this bug doesn't manifest anymore.

Do you have a timeline already on triaging and fixing this bug? By default we release the information after 30 days unless there are good reasons to delay the disclosure.

Magnus, can you find someone to backport the relevant patches from upstream?

I've been using this patch to apply parser_get_next_char from latest upstream:

--- comm/calendar/libical/src/libical/icalparser.c      2019-05-28 09:12:25.577672451 +0000
+++ comm/calendar/libical/src/libical/icalparser.c      2019-05-28 09:15:01.604897470 +0000
@@ -190,25 +190,24 @@
 char* parser_get_next_char(char c, char *str, int qm)
 {
     int quote_mode = 0;
-    char* p;
+    char *p = str;
+    char next_char = *p;
+    char prev_char = 0;
 
-    for(p=str; *p!=0; p++){
-           if (qm == 1) {
-                               if ( quote_mode == 0 && *p=='"' && *(p-1) != '\\' ){
-                                               quote_mode =1;
-                                               continue;
-                               }
-
-                               if ( quote_mode == 1 && *p=='"' && *(p-1) != '\\' ){
-                                               quote_mode =0;
-                                               continue;
-                               }
-           }
-
-               if (quote_mode == 0 &&  *p== c  && *(p-1) != '\\' ){
-                               return p;
-               } 
+    while (next_char != '\0') {
+        if ((prev_char != '\0') && (prev_char != '\\')) {
+            if (qm == 1 && next_char == '"') {
+                /* Encountered a quote, toggle quote mode */
+                quote_mode = !quote_mode;
+            } else if (quote_mode == 0 && next_char == c) {
+                /* Found a matching character out of quote mode, return it */
+                return p;
+            }
+        }
 
+        /* Save the previous character so we can check if it's a backslash in the next iteration */
+        prev_char = next_char;
+        next_char = *(++p);
     }
 
     return 0;

calling sec-high assuming this can be triggered in Thunderbird from an .ics mail attachment, but I haven't tried to see if the attached POC can be made to do that.

Made that a proper hg patch.

Looks like upstream: https://github.com/libical/libical/blob/e84714e9d6ec724dca889531e11fb963cadc2dba/src/libical/icalparser.c#L147

Minusing for Mozilla bounty as Thunderbird and items relating to it are not part of our bounty program.

(In reply to Magnus Melin [:mkmelin] from comment #6)

Made that a proper hg patch.

It would have been nice if you had provided a commit message relating to this bug here and not bug 1553808 :-(

https://hg.mozilla.org/comm-central/rev/383efc5887ecf146ecfb8f5e5853b5869242e27b
Heap buffer overflow in icalparser.c parser_get_next_char. r=philipp

https://hg.mozilla.org/comm-central/rev/f4628616dd10a3b267b6c2c9540b7492ab64f6fa
Backed out changeset 383efc5887ec (bug 1553820) for failure in test_bug1209399.js. a=backout DONTBUILD

(In reply to Jorg K (GMT+2) from comment #11)

https://hg.mozilla.org/comm-central/rev/f4628616dd10a3b267b6c2c9540b7492ab64f6fa
Backed out changeset 383efc5887ec (bug 1553820) for failure in test_bug1209399.js. a=backout DONTBUILD

Sadly, nothing to say here.

Someone needs to debug why the test fails with the patch.

I wonder if the difference between "" and null even matters here. Philipp?

(In reply to Geoff Lankow (:darktrojan) from comment #14)

I wonder if the difference between "" and null even matters here. Philipp?

I'm guessing the check used to be ===, but equal now just does ==, so no difference at the moment.

I'm assuming Magnus or Geoff have the engineering work here covered, clearing my NI.

(In reply to Jorg K (GMT+2) from comment #10)

https://hg.mozilla.org/comm-central/rev/383efc5887ecf146ecfb8f5e5853b5869242e27b
Heap buffer overflow in icalparser.c parser_get_next_char. r=philipp

Since the patch is already public with references to this bug, we will publish a short writeup in the next days.

Many thanks for addressing this so quickly.

There seems to be a bug in libical that prevents things happening on the first character in str. (Compare this patch to the last.) I'm not imagining that, am I?

Luis, can you check this patch still solves the reported problem? I think it should be fine, but just to be sure…

For convenience: https://bugzilla.mozilla.org/attachment.cgi?oldid=9069087&action=interdiff&newid=9070118&headers=1

I will, but I wanted to check I was actually right first.

Edit: https://github.com/libical/libical/pull/395

(In reply to Geoff Lankow (:darktrojan) from comment #17)

Created attachment 9070118 [details] [diff] [review]
1553820-1.diff

There seems to be a bug in libical that prevents things happening on the first character in str. (Compare this patch to the last.) I'm not imagining that, am I?

Luis, can you check this patch still solves the reported problem? I think it should be fine, but just to be sure…

After applying your patch, a new heap overflow reading out of bounds appears:

2019-06-06_13:24:24.85446 INFO:dasfuzzer.cpu1:==29877==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6070016e89cf at pc 0x00000051b285 bp 0x7ffdee9df010 sp 0x7ffdee9df008
2019-06-06_13:24:24.85458 INFO:dasfuzzer.cpu1:READ of size 1 at 0x6070016e89cf thread T0
2019-06-06_13:24:24.87828 INFO:dasfuzzer.cpu1:#0 0x51b284 in parser_get_next_value /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:385:19
2019-06-06_13:24:24.87841 INFO:dasfuzzer.cpu1:#1 0x51a6a5 in icalparser_add_line /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:1053:24
2019-06-06_13:24:24.87854 INFO:dasfuzzer.cpu1:#2 0x518df7 in icalparser_parse /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:622:11
2019-06-06_13:24:24.87866 INFO:dasfuzzer.cpu1:#3 0x4fe242 in icalparser_parse_string /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:1235:9
2019-06-06_13:24:24.87880 INFO:dasfuzzer.cpu1:#4 0x4fb9d5 in LLVMFuzzerTestOneInput /opt/src/fuzzer.cc:13:38
2019-06-06_13:24:24.87893 INFO:dasfuzzer.cpu1:#5 0x43b7c1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/opt/out/thunderbird_libical_fuzzer+0x43b7c1)
2019-06-06_13:24:24.87916 INFO:dasfuzzer.cpu1:#6 0x43aef5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/opt/out/thunderbird_libical_fuzzer+0x43aef5)
2019-06-06_13:24:24.87938 INFO:dasfuzzer.cpu1:#7 0x43d8fe in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/opt/out/thunderbird_libical_fuzzer+0x43d8fe)
2019-06-06_13:24:24.87950 INFO:dasfuzzer.cpu1:#8 0x43df00 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/opt/out/thunderbird_libical_fuzzer+0x43df00)
2019-06-06_13:24:24.87962 INFO:dasfuzzer.cpu1:#9 0x42aa89 in fuzzer::FuzzerDriver(int*, char***, int ()(unsigned char const, unsigned long)) (/opt/out/thunderbird_libical_fuzzer+0x42aa89)
2019-06-06_13:24:24.87973 INFO:dasfuzzer.cpu1:#10 0x455002 in main (/opt/out/thunderbird_libical_fuzzer+0x455002)
2019-06-06_13:24:24.92405 INFO:dasfuzzer.cpu1:#11 0x7f1c84f1cb96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
2019-06-06_13:24:24.92431 INFO:dasfuzzer.cpu1:#12 0x41dc99 in _start (/opt/out/thunderbird_libical_fuzzer+0x41dc99)
2019-06-06_13:24:24.92436 INFO:dasfuzzer.cpu1:
2019-06-06_13:24:24.92440 INFO:dasfuzzer.cpu1:0x6070016e89cf is located 1 bytes to the left of 80-byte region [0x6070016e89d0,0x6070016e8a20)
2019-06-06_13:24:24.92443 INFO:dasfuzzer.cpu1:allocated by thread T0 here:
2019-06-06_13:24:24.92455 INFO:dasfuzzer.cpu1:#0 0x4cce6d in malloc (/opt/out/thunderbird_libical_fuzzer+0x4cce6d)
2019-06-06_13:24:24.92467 INFO:dasfuzzer.cpu1:#1 0x4ff7bc in icalmemory_new_buffer /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalmemory.c:266:15
2019-06-06_13:24:24.92480 INFO:dasfuzzer.cpu1:#2 0x518182 in icalparser_get_line /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:452:21
2019-06-06_13:24:24.92491 INFO:dasfuzzer.cpu1:#3 0x518de9 in icalparser_parse /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:620:13
2019-06-06_13:24:24.92503 INFO:dasfuzzer.cpu1:#4 0x4fe242 in icalparser_parse_string /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:1235:9
2019-06-06_13:24:24.92514 INFO:dasfuzzer.cpu1:#5 0x4fb9d5 in LLVMFuzzerTestOneInput /opt/src/fuzzer.cc:13:38
2019-06-06_13:24:24.92526 INFO:dasfuzzer.cpu1:#6 0x43b7c1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/opt/out/thunderbird_libical_fuzzer+0x43b7c1)
2019-06-06_13:24:24.92537 INFO:dasfuzzer.cpu1:#7 0x43aef5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/opt/out/thunderbird_libical_fuzzer+0x43aef5)
2019-06-06_13:24:24.92549 INFO:dasfuzzer.cpu1:#8 0x43d8fe in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/opt/out/thunderbird_libical_fuzzer+0x43d8fe)
2019-06-06_13:24:24.92560 INFO:dasfuzzer.cpu1:#9 0x43df00 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/opt/out/thunderbird_libical_fuzzer+0x43df00)
2019-06-06_13:24:24.92571 INFO:dasfuzzer.cpu1:#10 0x42aa89 in fuzzer::FuzzerDriver(int*, char***, int ()(unsigned char const, unsigned long)) (/opt/out/thunderbird_libical_fuzzer+0x42aa89)
2019-06-06_13:24:24.92582 INFO:dasfuzzer.cpu1:#11 0x455002 in main (/opt/out/thunderbird_libical_fuzzer+0x455002)
2019-06-06_13:24:24.92598 INFO:dasfuzzer.cpu1:#12 0x7f1c84f1cb96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
2019-06-06_13:24:24.92601 INFO:dasfuzzer.cpu1:
2019-06-06_13:24:24.93552 INFO:dasfuzzer.cpu1:SUMMARY: AddressSanitizer: heap-buffer-overflow /opt/src/thunderbird-60.6.1/comm/calendar/libical/src/libical/icalparser.c:385:19 in parser_get_next_value

I think this is right, but upstream libical still has the same code that we do, so I'm suspicious of this change.

Geoff, could you please clarify the status of each of the attached patches?
I don't understand which patches need to be checked in.

The first was checked in, but backed out.

The second doesn't have review.

The third has review.

Could you please mark patches as "obsolete", that shouldn't get checked in?
Could you please get review on the patches that need to get checked in?

ok, the first one isn't a patch, we have only two patches.

What needs to get checked in?

Also, please request approval for esr and beta for the patches that need to be uplifted.

https://hg.mozilla.org/comm-central/rev/bab0908888b8a7f8399a440bbc345b3d895d68c4
https://hg.mozilla.org/comm-central/rev/e48bbc985397423402a45e5fbf0b63b46e7470cf

TB 68 beta / Cal 7.0:
https://hg.mozilla.org/releases/comm-beta/rev/c40ad6602e65fa63674ee9c3cce0ca26fa96104a
https://hg.mozilla.org/releases/comm-beta/rev/e2772684f3c650409bb84b65407b56cf3c98d285

TB 60.7.1 ESR / Cal 6.2.7.1:
https://hg.mozilla.org/releases/comm-esr60/rev/54655ef5946473058177a8a6223c8111a676bcc7
https://hg.mozilla.org/releases/comm-esr60/rev/55c22d6687db36bbf199c813a9f8ddfa9c22c72c

I'd be interested why this bug is bounty eligible now, but it wasn't three years ago when I reported it (as well as the handful of others I reported at the same time).

Hi Brandon. IIUC there isn't a bug bounty for Thunderbird issues.

It's been flagged as sec-bounty?

I see above Thunderbird is indeed not. Thanks for the clarification.

If there would be a bounty, the bounty should definitely go to Brandon for the bugs he reported three years ago. Especially if they were reported at a time where the bounty for TB was still in effect?

Per this comment by abillings 3 years ago, the reason these were not bountied is because they marked them as sec-low. Not because TB wasn't under the bounty.

https://bugzilla.mozilla.org/show_bug.cgi?id=1280832#c24

Now that we have decided they are indeed high-severity, I'd like to see these reconsidered.

The silence is deafening.

(In reply to Brandon Perry from comment #41)

The silence is deafening.

Leaving a comment here without a ni? is unlikely to be seen quickly. I hadn't seen your previous comment until you set the ni? today. (Many of us get 100s of emails for bugs a day.)

If you email security@mozilla.org, you can be guaranteed a quicker response. I'll mark this for bounty consideration in the meantime.

In comment 5 I guessed "sec-high assuming this can be triggered in Thunderbird from an .ics mail attachment". Has that been tested/demonstrated? We called bug 1281041 sec-low because we thought not, so which is right?

(In reply to Daniel Veditz [:dveditz] from comment #43)

In comment 5 I guessed "sec-high assuming this can be triggered in Thunderbird from an .ics mail attachment". Has that been tested/demonstrated? We called bug 1281041 sec-low because we thought not, so which is right?

This ical file will crash an ASAN enabled Thunderbird and given the right conditions (heap layout needs to be massaged) also production Thunderbird:

https://github.com/x41sec/advisories/blob/master/X41-2019-002/x41-2019-002-thunderbird.ical

Awarding a bounty for this since when it was first reported in bug 1281041 Thunderbird was covered by the bounty program. Splitting the bounty between the two bugs.

I appreciate the reconsideration. I'll be donating the bounty to Austin Pets Alive.