Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A heap-buffer-overflow in icaltime_from_string #251

Closed
agustinmista opened this issue Dec 2, 2016 · 14 comments
Closed

A heap-buffer-overflow in icaltime_from_string #251

agustinmista opened this issue Dec 2, 2016 · 14 comments
Assignees
Milestone

Comments

@agustinmista
Copy link

agustinmista commented Dec 2, 2016

Hello, we recently found a memory issue parsing and executing fuzzed ical file in last revision of libical (#19acf43794ad4c99f7e6687cb39424a82b737828).
We tested this issue on Ubuntu 14.04 but other configurations could be affected.
Technical details about the issue are:

==8557==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300000caa3 at pc 0x7ffff7b187ce bp 0x7fffffff87f0 sp 0x7fffffff87e8
READ of size 1 at 0x60300000caa3 thread T0

gdb backtrace is as follows:

#0  0x00007ffff61cfc37 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff61d3028 in __GI_abort () at abort.c:89
#2  0x00000000004b12b6 in __sanitizer::Abort() ()
#3  0x00000000004a1f97 in __asan::AsanDie() ()
#4  0x00000000004a89cf in __sanitizer::Die() ()
#5  0x00000000004a062b in __asan::ScopedInErrorReport::~ScopedInErrorReport() ()
#6  0x00000000004a0171 in __asan_report_error ()
#7  0x00000000004a0db7 in __asan_report_load1 ()
#8  0x00007ffff7b187ce in icaltime_from_string (str=<optimized out>)
    at /home/agustin/Code/libical/src/libical/icaltime.c:448
#9  0x00007ffff7b5756f in icalvalue_new_from_string_with_error (kind=<optimized out>, 
    str=<optimized out>, error=<optimized out>)
    at /home/agustin/Code/libical/src/libical/icalvalue.c:637
#10 0x00007ffff7b548db in icalvalue_new_from_string (kind=ICAL_DATETIME_VALUE, 
    str=0x60300000ca90 "18640529T011608z")
    at /home/agustin/Code/libical/src/libical/icalvalue.c:756
#11 0x00007ffff7ad080a in icalparser_add_line (parser=<optimized out>, 
    line=<optimized out>) at /home/agustin/Code/libical/src/libical/icalparser.c:1147
#12 0x00000000004b8c3f in main (argc=2, argv=0x7fffffffdf08)
    at /home/agustin/Code/libical/src/test/icaltestparser.c:104

This issue was found using QuickFuzz, the file to reproduce it is attached.
Regards.

@winterz winterz added this to the 2.1.0 milestone Dec 2, 2016
@winterz winterz self-assigned this Dec 2, 2016
@ksmurchison
Copy link
Contributor

Can you attach icaltestparser.c so I don't have to write my own test harness? I believe I already have a fix for this.

@ksmurchison
Copy link
Contributor

Never mind. I didn't realize you were using the harness in libical

@ksmurchison
Copy link
Contributor

Proposed patch:

diff --git a/src/libical/icaltime.c b/src/libical/icaltime.c
index ca64763..4077ce7 100644
--- a/src/libical/icaltime.c
+++ b/src/libical/icaltime.c
@@ -445,7 +445,7 @@ struct icaltimetype icaltime_from_string(const char str)
tt.is_utc = 0;
tt.is_date = 0;
} else if ((size == 16) || (size == 20)) { /
UTC time, ends in 'Z' */

  •    if ((str[15] != 'Z') && (str[19] != 'Z'))
    
  •    if ((str[size-1] != 'Z'))
           goto FAIL;
    
       tt.is_utc = 1;
    

@agustinmista
Copy link
Author

Markdown seems to have mangled your patch, can you please fix it?

@ksmurchison
Copy link
Contributor

icaltime.c.diff.txt

@winterz
Copy link
Member

winterz commented Dec 2, 2016

verified that Ken's patch fixes the overflow. but now we get stuck in an infinite loop in icalparser

               /* Find the last colon in the line */
                do {
                    nextColon = parser_get_next_char(':', nextColon, 1);

                    if (nextColon) {
                        lastColon = nextColon;
                    }
                } while (nextColon);

@ksmurchison
Copy link
Contributor

icalparser.c.diff.txt

@ksmurchison
Copy link
Contributor

This fixes the infinite loop. Note that icaltestparser.c leaks the parsed component(s) (verified by Valgrind)

@ksmurchison
Copy link
Contributor

I just committed a fix to icaltestparser.c that fixes the stuff that it was leaking directly. A component is still being leaked when the iCalendar is incomplete/bogus. Still tracking this one down.

Should I commit my icaltime.c and icalparser.c fixes?

@winterz
Copy link
Member

winterz commented Dec 2, 2016

yes please commit. And you can close this issue too.

@ksmurchison
Copy link
Contributor

Fixed with commits:
38757ab
04d8474

@winterz
Copy link
Member

winterz commented Dec 2, 2016

hehe.. I see the leak and coverity had found it previously. and I cheated and added a /* coverity[leaked_storage] */ comment so Coverity wouldn't complain. yeah, if you nice if you can fix that leak.

@ksmurchison
Copy link
Contributor

The leak wasn't where you thought. It was easy to find once I compiled a debug version of libical (I was guessing before)

830d953

@rhertzog
Copy link

Just to cross-reference data, this is supposed to be CVE-2016-5824 that was also reported to mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1275400

This is according to http://www.openwall.com/lists/oss-security/2017/01/20/16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants