Overflow prevention #3878
Overflow prevention #3878
Conversation
crypto/bn/bn_print.c
Outdated
| @@ -67,9 +67,10 @@ char *BN_bn2dec(const BIGNUM *a) | |||
| */ | |||
| i = BN_num_bits(a) * 3; | |||
| num = (i / 10 + i / 1000 + 1) + 1; | |||
| tbytes = num + 3; | |||
There was a problem hiding this comment.
why +3? A comment here would help.
There was a problem hiding this comment.
This was in the original code in the malloc which was comment free.
Negative sign and null terminator are two. I've no idea why 3.
The derivation of the bound on num in the comment above looks correct.
crypto/pem/pem_lib.c
Outdated
| @@ -71,6 +71,7 @@ int PEM_def_callback(char *buf, int num, int w, void *key) | |||
| void PEM_proc_type(char *buf, int type) | |||
| { | |||
| const char *str; | |||
| char *p = strchr(buf, '\0'); | |||
There was a problem hiding this comment.
is this anything other than buf+strlen(buf) ?
There was a problem hiding this comment.
Nope, they do the same thing.
Want these changed?
crypto/pem/pem_lib.c
Outdated
| } | ||
| buf[j + i * 2] = '\n'; | ||
| buf[j + i * 2 + 1] = '\0'; | ||
| char *p = strchr(buf, '\0'); |
crypto/pem/pem_lib.c
Outdated
| buf[j + i * 2] = '\n'; | ||
| buf[j + i * 2 + 1] = '\0'; | ||
| char *p = strchr(buf, '\0'); | ||
| int j = PEM_BUFSIZE - (p - buf), n; |
There was a problem hiding this comment.
int cast on the pointer subtraction
crypto/pem/pem_lib.c
Outdated
| p += n; | ||
| } | ||
| if (j > 1) | ||
| memcpy(p, "\n", sizeof("\n")); |
There was a problem hiding this comment.
strcpy(p, "\n") seems more clear.
| @@ -570,14 +573,14 @@ static int load_iv(char **fromp, unsigned char *to, int num) | |||
| v = OPENSSL_hexchar2int(*from); | |||
| if (v < 0) { | |||
| PEMerr(PEM_F_LOAD_IV, PEM_R_BAD_IV_CHARS); | |||
| return (0); | |||
| return 0; | |||
| } | |||
| from++; | |||
| to[i / 2] |= v << (long)((!(i & 1)) * 4); | |||
There was a problem hiding this comment.
side note: whoever wrote that expression should be smacked.
There was a problem hiding this comment.
Agree. I didn't notice it.
|
Yes, please change them; strlen is more obvious.
|
|
Okay fine
|
|
Changes are in. |
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3878)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3878)
Addressing strcpy, strcat and sprintf issues in
crypto/bn/bn_print.c,crypto/mem_dbg.candcrypto/pem/pem_lib.cAlso replace
return (x)withreturn x