stack out of bounds read in function iterate_rc4 #147

hannob · Aug 23, 2017

The attached file will cause an out of bounds read in qpdf, detectable with address sanitizer.
qpdf-stack-oob-iterate_rc4.zip

==16591==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffee632f740 at pc 0x00000075ee62 bp 0x7ffee632f390 sp 0x7ffee632f388
READ of size 1 at 0x7ffee632f740 thread T0
    #0 0x75ee61 in iterate_rc4(unsigned char*, int, unsigned char*, int, int, bool) /f/qpdf/qpdf/libqpdf/QPDF_encryption.cc:211:15
    #1 0x731971 in check_owner_password_V4(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, QPDF::EncryptionData const&) /f/qpdf/qpdf/libqpdf/QPDF_encryption.cc:594:5
    #2 0x731971 in check_owner_password(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, QPDF::EncryptionData const&) /f/qpdf/qpdf/libqpdf/QPDF_encryption.cc:628
    #3 0x731971 in QPDF::initializeEncryption() /f/qpdf/qpdf/libqpdf/QPDF_encryption.cc:1014
    #4 0x568e6c in QPDF::parse(char const*) /f/qpdf/qpdf/libqpdf/QPDF.cc:343:5
    #5 0x565e1a in QPDF::processFile(char const*, char const*) /f/qpdf/qpdf/libqpdf/QPDF.cc:141:5
    #6 0x51853d in main /f/qpdf/qpdf/qpdf/qpdf.cc:2300:17
    #7 0x7f95a13f14f0 in __libc_start_main (/lib64/libc.so.6+0x204f0)
    #8 0x41d459 in _start (/f/qpdf/qpdf/qpdf/build/qpdf+0x41d459)

Address 0x7ffee632f740 is located in stack of thread T0 at offset 320 in frame
    #0 0x7273cf in QPDF::initializeEncryption() /f/qpdf/qpdf/libqpdf/QPDF_encryption.cc:785

  This frame has 146 object(s):
    [32, 40) '__dnew.i.i.i.i3390'
    [64, 72) '__dnew.i.i.i.i3372'
    [96, 97) 'disregard.i.i'
    [112, 120) '__dnew.i.i.i.i3318'
    [144, 176) 'u_value.i.i'
    [208, 240) 'u_value.i.i.i.i'
    [272, 280) '__dnew.i.i.i.i.i.i'
    [304, 320) 'key.i.i' <== Memory access at offset 320 overflows this variable
    [336, 368) 'O_data.i.i'
    [400, 432) 'new_user_password.i.i'
    [464, 472) '__dnew.i.i.i.i2662'
    [496, 504) '__dnew.i.i.i.i2627'
    [528, 536) '__dnew.i.i.i.i2561'
    [560, 568) '__dnew.i.i.i.i2543'
    [592, 600) '__dnew.i.i.i.i2372'
    [624, 632) '__dnew.i.i.i.i2354'
    [656, 664) '__dnew.i.i.i.i2102'
    [688, 696) '__dnew.i.i.i.i2084'
    [720, 728) '__dnew.i.i.i.i1953'
    [752, 760) '__dnew.i.i.i.i1935'
    [784, 792) '__dnew.i.i.i.i1760'
    [816, 824) '__dnew.i.i.i.i1454'
    [848, 856) '__dnew.i.i.i.i1436'
    [880, 888) '__dnew.i.i.i.i1206'
    [912, 920) '__dnew.i.i.i.i1188'
    [944, 952) '__dnew.i.i.i.i1081'
    [976, 984) '__dnew.i.i.i.i1063'
    [1008, 1016) '__dnew.i.i.i.i962'
    [1040, 1048) '__dnew.i.i.i.i902'
    [1072, 1104) 'ref.tmp'
    [1136, 1168) 'id1'
    [1200, 1240) 'id_obj'
    [1280, 1312) 'ref.tmp14'
    [1344, 1384) 'temp.lvalue'
    [1424, 1456) 'ref.tmp20'
    [1488, 1528) 'temp.lvalue21'
    [1568, 1696) 'ref.tmp23'
    [1728, 1760) 'ref.tmp28'
    [1792, 1824) 'ref.tmp35'
    [1856, 1896) 'encryption_dict'
    [1936, 1968) 'ref.tmp40'
    [2000, 2032) 'ref.tmp57'
    [2064, 2104) 'temp.lvalue60'
    [2144, 2176) 'ref.tmp61'
    [2208, 2240) 'ref.tmp64'
    [2272, 2312) 'temp.lvalue66'
    [2352, 2384) 'ref.tmp67'
    [2416, 2448) 'ref.tmp99'
    [2480, 2512) 'ref.tmp106'
    [2544, 2584) 'temp.lvalue110'
    [2624, 2656) 'ref.tmp111'
    [2688, 2816) 'ref.tmp115'
    [2848, 2880) 'ref.tmp123'
    [2912, 2944) 'ref.tmp130'
    [2976, 3016) 'temp.lvalue132'
    [3056, 3088) 'ref.tmp133'
    [3120, 3160) 'temp.lvalue136'
    [3200, 3232) 'ref.tmp137'
    [3264, 3304) 'temp.lvalue145'
    [3344, 3376) 'ref.tmp146'
    [3408, 3448) 'temp.lvalue154'
    [3488, 3520) 'ref.tmp155'
    [3552, 3592) 'temp.lvalue163'
    [3632, 3664) 'ref.tmp164'
    [3696, 3728) 'ref.tmp221'
    [3760, 3792) 'ref.tmp228'
    [3824, 3864) 'temp.lvalue232'
    [3904, 3936) 'ref.tmp233'
    [3968, 4008) 'temp.lvalue236'
    [4048, 4080) 'ref.tmp237'
    [4112, 4144) 'O'
    [4176, 4216) 'temp.lvalue241'
    [4256, 4288) 'ref.tmp242'
    [4320, 4352) 'U'
    [4384, 4424) 'temp.lvalue244'
    [4464, 4496) 'ref.tmp245'
    [4528, 4568) 'temp.lvalue247'
    [4608, 4640) 'ref.tmp248'
    [4672, 4704) 'ref.tmp266'
    [4736, 4768) 'ref.tmp273'
    [4800, 4832) 'ref.tmp274'
    [4864, 4896) 'ref.tmp275'
    [4928, 4960) 'ref.tmp276'
    [4992, 5024) 'ref.tmp277'
    [5056, 5088) 'ref.tmp279'
    [5120, 5152) 'OE'
    [5184, 5216) 'UE'
    [5248, 5280) 'Perms'
    [5312, 5344) 'ref.tmp300'
    [5376, 5408) 'ref.tmp307'
    [5440, 5480) 'temp.lvalue311'
    [5520, 5552) 'ref.tmp312'
    [5584, 5624) 'temp.lvalue315'
    [5664, 5696) 'ref.tmp316'
    [5728, 5768) 'temp.lvalue324'
    [5808, 5840) 'ref.tmp325'
    [5872, 5904) 'ref.tmp362'
    [5936, 5968) 'ref.tmp369'
    [6000, 6032) 'ref.tmp373'
    [6064, 6104) 'temp.lvalue374'
    [6144, 6176) 'ref.tmp375'
    [6208, 6240) 'ref.tmp378'
    [6272, 6312) 'temp.lvalue379'
    [6352, 6384) 'ref.tmp380'
    [6416, 6448) 'ref.tmp383'
    [6480, 6520) 'temp.lvalue384'
    [6560, 6592) 'ref.tmp385'
    [6624, 6656) 'ref.tmp406'
    [6688, 6720) 'ref.tmp413'
    [6752, 6792) 'temp.lvalue417'
    [6832, 6864) 'ref.tmp418'
    [6896, 6936) 'temp.lvalue421'
    [6976, 7008) 'ref.tmp422'
    [7040, 7072) 'ref.tmp437'
    [7104, 7136) 'ref.tmp444'
    [7168, 7208) 'temp.lvalue451'
    [7248, 7280) 'ref.tmp452'
    [7312, 7352) 'temp.lvalue470'
    [7392, 7424) 'ref.tmp471'
    [7456, 7496) 'CF'
    [7536, 7568) 'ref.tmp479'
    [7600, 7648) 'keys'
    [7680, 7720) 'cdict'
    [7760, 7800) 'temp.lvalue488'
    [7840, 7872) 'ref.tmp489'
    [7904, 7936) 'method_name'
    [7968, 8008) 'temp.lvalue492'
    [8048, 8080) 'ref.tmp493'
    [8112, 8152) 'StmF'
    [8192, 8224) 'ref.tmp502'
    [8256, 8296) 'StrF'
    [8336, 8368) 'ref.tmp504'
    [8400, 8440) 'EFF'
    [8480, 8512) 'ref.tmp506'
    [8544, 8584) 'agg.tmp'
    [8624, 8664) 'agg.tmp511'
    [8704, 8744) 'agg.tmp516'
    [8784, 9000) 'data'
    [9072, 9104) 'ref.tmp554'
    [9136, 9168) 'ref.tmp556'
    [9200, 9232) 'ref.tmp561'
    [9264, 9265) 'perms_valid'
    [9280, 9312) 'ref.tmp568'
    [9344, 9472) 'ref.tmp577'
    [9504, 9536) 'ref.tmp585'
    [9568, 9600) 'ref.tmp592'

Thanks. I am able to reproduce this. I will make sure it is fixed before 7.0.0 is released.

jberkenbilt · Aug 23, 2017

Thanks. I am able to reproduce this. I will make sure it is fixed before 7.0.0 is released.

I looked through the code, and there are places where I'm passing buffers that are too short. I'll audit all the calls to iterate_rc4 and make sure this is fixed properly.

jberkenbilt · Aug 26, 2017

I looked through the code, and there are places where I'm passing buffers that are too short. I'll audit all the calls to iterate_rc4 and make sure this is fixed properly.

I've fixed this one and am now going through and making sure qpdf's test suite is clean with address sanitizer. That was the only invalid read. There are several small leaks, though most are not for situations that would occur under ordinary use. Still, they all have to be fixed, and right now, they all look like they'll be easy to fix.

jberkenbilt · Aug 27, 2017

I've fixed this one and am now going through and making sure qpdf's test suite is clean with address sanitizer. That was the only invalid read. There are several small leaks, though most are not for situations that would occur under ordinary use. Still, they all have to be fixed, and right now, they all look like they'll be easy to fix.

Actually, address sanitizer gets false positives with PointerHolder. valgrind doesn't show any leaks throughout the entire test suite. Address sanitizer doesn't show any errors except leaks, and based on my analysis with a quick script, every direct leak reported by address sanitizer has PointerHolder in its stack trace. There was actually only one exception, which was a missing free() call in the test suite for the C API. It was just test code. I fixed it. The actual library isn't leaking anything.

So I think there are no actual memory leaks for any situation exercised by the test suite at this point. That should be most of them. I'm not sure why address sanitizer is reporting leaks with PointerHolder. I've been using that code since around 1997 including on long-running systems. I tried quickly to reproduce it, but my first attempt didn't work.

Anyway, thanks for this report. I think we should be in good shape overall with this type of issue.

jberkenbilt · Aug 27, 2017

Actually, address sanitizer gets false positives with PointerHolder. valgrind doesn't show any leaks throughout the entire test suite. Address sanitizer doesn't show any errors except leaks, and based on my analysis with a quick script, every direct leak reported by address sanitizer has PointerHolder in its stack trace. There was actually only one exception, which was a missing free() call in the test suite for the C API. It was just test code. I fixed it. The actual library isn't leaking anything.

So I think there are no actual memory leaks for any situation exercised by the test suite at this point. That should be most of them. I'm not sure why address sanitizer is reporting leaks with PointerHolder. I've been using that code since around 1997 including on long-running systems. I tried quickly to reproduce it, but my first attempt didn't work.

Anyway, thanks for this report. I think we should be in good shape overall with this type of issue.

I'll have a look at the leak. It is very rare that asan produces false positives (and if it does I'd rather like to report them to the asan devs).

Other than that: If you're interested here's the code stub I used to test qpdf with libfuzzer:
https://github.com/hannob/libfuzzer-examples/blob/master/libfuzzer-qpdf.cpp

libfuzzer stops with every bug found, so for proper fuzzing it required all the easy to find bugs I reported lately to be fixed first.

Also I'd recommend that you add all the malformed PDFs I sent you in the bug reports to the test suite to make sure they're properly archived for future testing. (I also recommend cross-testing with known bug-triggering samples of different PDF implementations, I'll write a blogpost about this soon.)

hannob · Aug 27, 2017

I'll have a look at the leak. It is very rare that asan produces false positives (and if it does I'd rather like to report them to the asan devs).

Other than that: If you're interested here's the code stub I used to test qpdf with libfuzzer:
https://github.com/hannob/libfuzzer-examples/blob/master/libfuzzer-qpdf.cpp

libfuzzer stops with every bug found, so for proper fuzzing it required all the easy to find bugs I reported lately to be fixed first.

Also I'd recommend that you add all the malformed PDFs I sent you in the bug reports to the test suite to make sure they're properly archived for future testing. (I also recommend cross-testing with known bug-triggering samples of different PDF implementations, I'll write a blogpost about this soon.)

I have added all but one of the PDFs to the test suite. I didn't add this one because it doesn't actually cause a test failure before the fix, though I should recheck that.

You can configure with asan and run the test suite. There are about 10 or 15 failures. Take a look at the leaks. valgrind does not recognize them as leaks. I'll see if I can narrow down to either reproduce a bug to report or figure out why they are actually leaks if they are.

I'll reopen until I've done this. Thanks again.

jberkenbilt · Aug 27, 2017

I have added all but one of the PDFs to the test suite. I didn't add this one because it doesn't actually cause a test failure before the fix, though I should recheck that.

You can configure with asan and run the test suite. There are about 10 or 15 failures. Take a look at the leaks. valgrind does not recognize them as leaks. I'll see if I can narrow down to either reproduce a bug to report or figure out why they are actually leaks if they are.

I'll reopen until I've done this. Thanks again.

I also didn't add 148. I'll add these and make sure I have a way to see test failures with them. I would like to make qpdf test clean with address sanitizer so I can have it turned on as a routine part of my development. I do test with valgrind as a routine part of my development, and qpdf tests clean with that.

jberkenbilt · Aug 27, 2017

I also didn't add 148. I'll add these and make sure I have a way to see test failures with them. I would like to make qpdf test clean with address sanitizer so I can have it turned on as a routine part of my development. I do test with valgrind as a routine part of my development, and qpdf tests clean with that.

It's possible that there may be leaks with PointerHolder if the object being stored throws an exception on its constructor. This idea just occurred to me as this is a common leak case in C++. I haven't actually checked. I'll give it a look. This is as much a reminder to myself as anything else.

jberkenbilt · Aug 27, 2017

It's possible that there may be leaks with PointerHolder if the object being stored throws an exception on its constructor. This idea just occurred to me as this is a common leak case in C++. I haven't actually checked. I'll give it a look. This is as much a reminder to myself as anything else.

I've had only 10 minutes to look at it, but there is definitely a leak. I will find and fix it.

jberkenbilt · Aug 28, 2017

I've had only 10 minutes to look at it, but there is definitely a leak. I will find and fix it.

I found the leak. There were circular references in stream dictionaries and involving stream providers. There was already a mechanism in qpdf to break circular references at the time of destruction, and it was already being used for arrays and dictionaries, but not for streams. It was a very small code change to break cycles for streams and it could be done without breaking binary compatibility. After that small fix, there are only three tests left with leaks. I will look at them now.

jberkenbilt · Aug 29, 2017

I found the leak. There were circular references in stream dictionaries and involving stream providers. There was already a mechanism in qpdf to break circular references at the time of destruction, and it was already being used for arrays and dictionaries, but not for streams. It was a very small code change to break cycles for streams and it could be done without breaking binary compatibility. After that small fix, there are only three tests left with leaks. I will look at them now.

qpdf's test suite is now clean with address santizer up through this issue, so I'm closing it again.

jberkenbilt · Aug 29, 2017

qpdf's test suite is now clean with address santizer up through this issue, so I'm closing it again.

This has been assigned CVE-2017-18184

ghost · Feb 14, 2018

This has been assigned CVE-2017-18184

jberkenbilt added bug next labels Aug 23, 2017

jberkenbilt closed this in dea704f Aug 27, 2017

jberkenbilt reopened this Aug 27, 2017

jberkenbilt closed this Aug 29, 2017

qpdf/qpdf

stack out of bounds read in function iterate_rc4 #147

hannob commented Aug 23, 2017

This comment has been minimized.

jberkenbilt commented Aug 23, 2017

jberkenbilt added bug next labels Aug 23, 2017

This comment has been minimized.

jberkenbilt commented Aug 26, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

jberkenbilt closed this in `dea704f` Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

This comment has been minimized.

hannob commented Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

jberkenbilt reopened this Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 28, 2017

This comment has been minimized.

jberkenbilt commented Aug 29, 2017

This comment has been minimized.

jberkenbilt commented Aug 29, 2017

jberkenbilt closed this Aug 29, 2017

This comment has been minimized.

ghost commented Feb 14, 2018

qpdf/qpdf

Join GitHub today

stack out of bounds read in function iterate_rc4 #147

Comments

hannob commented Aug 23, 2017

This comment has been minimized.

jberkenbilt commented Aug 23, 2017

jberkenbilt added bug next labels Aug 23, 2017

This comment has been minimized.

jberkenbilt commented Aug 26, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

jberkenbilt closed this in dea704f Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

This comment has been minimized.

hannob commented Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

jberkenbilt reopened this Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 27, 2017

This comment has been minimized.

jberkenbilt commented Aug 28, 2017

This comment has been minimized.

jberkenbilt commented Aug 29, 2017

This comment has been minimized.

jberkenbilt commented Aug 29, 2017

jberkenbilt closed this Aug 29, 2017

This comment has been minimized.

ghost commented Feb 14, 2018

jberkenbilt closed this in `dea704f` Aug 27, 2017