libtasn1 with a fine-toothed comb

[Pascal Cuoq]

Hello,

I am currently giving libtasn1 the same treatment that John Regehr describes 
applying to SQLite here:
http://blog.regehr.org/archives/1292

This message is about the first two findings:


- One of the things that tis-interpreter has found so far is fprintf("%02x… 
being applied to what is, after integer promotions, an int. This pattern is 
fixed by the following commit:

https://github.com/pascal-cuoq/libtasn1-fork/commit/0803a38108e76ede7d4cd03dae0fcc713fdc7da9


- Another pattern has to do with memcpy() and length arguments of zero. Anyone 
who has looked into this recent commit knowns that C compilers have taken to 
optimize on the basis of a sentence in the C standard that says that the 
arguments of standard functions must not be “invalid values”:

“If an argument to a function has an invalid value (such as a value outside the 
domain of the function, or a pointer outside the address space of the program, 
or a null pointer, or a pointer to non-modifiable storage when the 
corresponding parameter is not const-qualified) or a type (after promotion) not 
expected by a function with variable number of arguments, the behavior is 
undefined.”

The above is in C11 (http://port70.net/~nsz/c/c11/n1570.html#7.1.4p1 ) but a 
similar sentence was present in C99. BIND started to be compiled to binaries 
that behaved incorrectly when GCC 4.9 introduced an optimization based on this 
sentence: http://blog.mycre.ws/articles/bind-and-gcc-49/

This clearly means that memcpy(NULL, p, 0) and memcpy(p, NULL, 0) are undefined 
behavior, or at least C compilers have decided that they are. It is debatable 
whether the same rule also means that pointers “one past the end” (t + n when t 
is an array of n elements) are invalid arguments for memcpy even together with 
a size of 0 
(http://stackoverflow.com/questions/25390577/is-memcpya-1-b-1-0-defined-in-c11 
) but in doubt I would recommend to future-proof the source code by avoiding 
such memcpy invocations.

tis-interpreter has found memcpy invoked with “one past the end” pointers in 
_asn1_append_value():
http://git.savannah.gnu.org/gitweb/?p=libtasn1.git;a=blob;f=lib/parser_aux.c;h=da9a388fe3204d22f56a138af319ee8a9b77d7f0;hb=bd6f37713d139b5d102df70248ee9af3422f0339#l304

When the function _asn1_append_value() is invoked with len containing 0, a 
memcpy call with a “one past the end” pointer can happen as a result either at 
line 330 or at line 346. Inputs to the commandline utility asn1Decoding that 
cause it to be called with 0 for the len argument causing this to happen.
If it is a logic error for _asn1_append_value() to be used this way, the 
invalid input can be diagnosed at any point before the function is called. Some 
call stacks ending in _asn1_append_value() being passed a length of zero are at 
the bottom of this message. The line numbers are those of version 4.7, possibly 
with small offsets when I needed to adapt files for tis-interpreter. The inputs 
are available on request.
If it is not a logic error for _asn1_append_value() to be used this way, then 
ideally the function should handle the case where len is 0 without calling 
memcpy on “one past the end” pointers to prevent future compiler optimizations 
to mess with that code.

Pascal

PS: I have used afl to generate inputs that exert (so far) 1206 different 
execution paths inside asn1Decode. I will make all the inputs available in 
batch at the end of the fuzzing session, but I would appreciate additional 
testcases, especially if they came in the form of inputs to asn1Decode.

--
                 assert((void *)(node->value + prev_len) is a valid pointer for 
writing)
                 stack: memcpy :: lib/parser_aux.c:330 <-
                        _asn1_append_value :: lib/decoding.c:788 <-
                        _asn1_extract_der_octet :: lib/decoding.c:885 <-
                        get_octet_string :: lib/decoding.c:1307 <-
                        asn1_der_decoding2 :: lib/decoding.c:1647 <-
                        asn1_der_decoding :: src/asn1Decoding.c:276 <-
                        simple_decode :: src/asn1Decoding.c:304 <-
                        decode :: src/asn1Decoding.c:225 <-
                        main

--
                 assert((void *)(node->value + prev_len_0) is a valid pointer 
for writing)
                 stack: memcpy :: lib/parser_aux.c:346 <-
                        _asn1_append_value :: lib/decoding.c:788 <-
                        _asn1_extract_der_octet :: lib/decoding.c:885 <-
                        get_octet_string :: lib/decoding.c:1307 <-
                        asn1_der_decoding2 :: lib/decoding.c:1647 <-
                        asn1_der_decoding :: src/asn1Decoding.c:276 <-
                        simple_decode :: src/asn1Decoding.c:304 <-
                        decode :: src/asn1Decoding.c:225 <-
                        main