search

avast / authenticode-parser

Authenticode-parser is a simple C library for Authenticode format parsing using OpenSSL.
MIT License
16 stars 8 forks source link

feat: use int32_t for len of data in authenticode_new #13

Closed vthib closed 1 year ago

vthib commented 1 year ago

As is done for the other entrypoint of the library parse_authenticode, use a signed large type for the length of the payload.

HoundThe commented 1 year ago

I am not sure if this is a good idea. I used long len as that is what OpenSSL entry point d2i_PKCS7() uses as I didn't want to create misunderstanding, that someone would want to fit something larger than long into the input length. OpenSSL entry would probably overflow and result in something undefined?

vthib commented 1 year ago

I see. I realize I didn't really explain why I was proposing this change:

I didn't track it all down to the d2i_PKCS7. Maybe the value could be clamped down to[0; LONG_MAX] to ensure there are no issues when passing the value (so using min(len, LONG_MAX)). Or, returning NULL when len > LONG_MAX.

I would favor the first solution myself, would you be ok with this?

HoundThe commented 1 year ago

The other entry point uses uint64_t not int64_t, mostly because it accepts an entire PE file, but honestly. I have some thoughts:

  1. By the limitation of PE format, the signature cannot be larger than 32bits - its length is stored in a 32bit variable (but unsigned)
  2. Honestly, no reasonable signature will be larger than a couple of kilobytes at most

I assume you want fixed-width data types due to the Rust bindings. I think having the limit in the function header is better than being hidden inside the function - the clamping. The symmetric with the entry point does look nice, but one is accepting the entire file, and the other accepts just the authenticode signature, so they are inherently different. I wouldn't necessarily try to have them the same argument types.

I would like a little more having the variable of type int32_t. It is transparent about the size limits, it is compatible with the long, because long has to be at least 32bits, and it has a fixed size, which is optimal for you. What do you think?

vthib commented 1 year ago

Yes sounds good! I updated the PR to use int32_t