Re: libtasn1 with a fine-toothed comb

[Nikos Mavrogiannopoulos]

On Sat, 2016-04-02 at 15:45 +0000, Pascal Cuoq wrote:
> 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

Applied, thank you.

> - 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.”
> 
[...]
> 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://s
> tackoverflow.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/parse
> r_aux.c;h=da9a388fe3204d22f56a138af319ee8a9b77d7f0;hb=bd6f37713d139b5
> d102df70248ee9af3422f0339#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.

Would you like to suggest a patch for this fix?

> 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.

I'm wondering, could afl be automated and used as part of the libtasn1
testsuite, or some extended test suite?

regards,
Nikos