Potential UB in secp256k1_der_parse_integer? #876

1. 
   practicalswift commented at 10:01 pm on January 23, 2021: contributor

   Do we consider memcpy(ra + 32 - rlen, *sig, rlen) in secp256k1_der_parse_integer to be defined in the case of rlen == 0?

   https://github.com/bitcoin-core/secp256k1/blob/98dac87839838b86094f1bccc71cc20e67b146cc/src/ecdsa_impl.h#L102-L104

   https://github.com/bitcoin-core/secp256k1/blob/98dac87839838b86094f1bccc71cc20e67b146cc/src/ecdsa_impl.h#L143

   Nothing high priority of course, but perhaps worth fixing? :)

   This code was introduced as part of PR #334 (“Overhaul ECDSA signature parsing: strict DER, compact sigs, tests, lower-S”) in 3bb9c447193a6d15726976c141f27d2c3ddd7991 back in 2015.

2. 
   sipa commented at 10:05 pm on January 23, 2021: contributor

   I don’t think there is any problem with that. memcpy is UB when called with invalid pointers even if length==0, but in this case I think the pointers are always valid.

   Edit: you’re right, this is technically UB. You can’t pass pointers to one-past-the-end of an array to memcpy.

3. 
   roconnor-blockstream commented at 0:38 am on January 24, 2021: contributor

   @sipa I’m not sure how you came to your conclusion:

   7.1.4.1 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. If a function argument is described as being an array, the pointer actually passed to the function shall have a value such that all address computations and accesses to objects (that would be valid if the pointer did point to the first element of such an array) are in fact valid.

   7.24.1 but in all cases a charor voidargument points to the initial (lowest addressed) character of the array. If an array is accessed beyond the end of an object, the behavior is undefined.

   The pointer being passed here is both a valid value, and all address computations and accesses to objects (of which there are none) are valid.

   By my reading, there is nothing wrong with the code. That said, while don’t think there is a need to change anything here, I’m not going to oppose anything.

4. 
   sipa commented at 1:13 am on January 24, 2021: contributor

   I consider all of this language-lawyering with very little impact on real systems, but at least here people argue that it violates the standard: https://stackoverflow.com/questions/29844298/is-it-legal-to-call-memcpy-with-zero-length-on-a-pointer-just-past-the-end-of-an
5. 
   roconnor-blockstream commented at 1:31 am on January 24, 2021: contributor

   The argument in your link seems fair.
6. sipa cross-referenced this on Jan 24, 2021 from issue Avoid passing out-of-bound pointers to 0-size memcpy by sipa
7. 
   practicalswift commented at 8:27 am on January 24, 2021: contributor

   […] at least here people argue that it violates the standard: https://stackoverflow.com/questions/29844298/is-it-legal-to-call-memcpy-with-zero-length-on-a-pointer-just-past-the-end-of-an

   This thread on the musl libc mailing list is also worth reading.

   Participants include:

   • Pascal Cuoq (author of Frama-C)
   • Rich Felkner (author of musl libc: the standards-conformant libc implementation)
   • Jens Gustedt (co-editor of the ISO C standard and author of the book Modern C)
8. real-or-random closed this on Jun 16, 2021

   ---

Contributors
practicalswift sipa roconnor-blockstream