hardening: negative snprintf return values #182
Conversation
| @@ -165,8 +165,10 @@ crypt_sha1crypt_rn (const char *phrase, size_t phr_size, | |||
| hmac_sha1_process_data (hmac_buf, SHA1_SIZE, pwu, pl, hmac_buf); | |||
| } | |||
| /* Now output... */ | |||
| pl = (size_t)snprintf ((char *)output, out_size, "%s%lu$%.*s$", | |||
| magic, iterations, (int)sl, setting); | |||
| dl = snprintf ((char *)output, out_size, "%s%lu$%.*s$", | |||
There was a problem hiding this comment.
I only looked in the ones that are casted to size_t, but you are right, it might make sense to go through all snprintf calls and make sure return value is checked for negativity.
There was a problem hiding this comment.
I think the iffiness is slightly above:
sl = (size_t)(sp - setting);Shouldn't this be bounded to a reasonable value? If I read the sources correctly, it currently isn't, although the documentation comment suggests it can be at most 64? Then the snprintf below cannot fail with EOVERFLOW (or otherwise) anymore.
There was a problem hiding this comment.
@fweimer-rh thanks for looking, I'll check.
| @@ -165,8 +165,10 @@ crypt_sha1crypt_rn (const char *phrase, size_t phr_size, | |||
| hmac_sha1_process_data (hmac_buf, SHA1_SIZE, pwu, pl, hmac_buf); | |||
| } | |||
| /* Now output... */ | |||
| pl = (size_t)snprintf ((char *)output, out_size, "%s%lu$%.*s$", | |||
| magic, iterations, (int)sl, setting); | |||
| dl = snprintf ((char *)output, out_size, "%s%lu$%.*s$", | |||
There was a problem hiding this comment.
I think the iffiness is slightly above:
sl = (size_t)(sp - setting);Shouldn't this be bounded to a reasonable value? If I read the sources correctly, it currently isn't, although the documentation comment suggests it can be at most 64? Then the snprintf below cannot fail with EOVERFLOW (or otherwise) anymore.
| size_t written = (size_t) snprintf ((char *)output, o_size, | ||
| "%s,rounds=%lu$", SUNMD5_PREFIX, count); | ||
| int written = snprintf ((char *)output, o_size, | ||
| "%s,rounds=%lu$", SUNMD5_PREFIX, count); |
There was a problem hiding this comment.
Is there an actual snprintf implementation that can fail with these inputs? I don't think so. There are many tricky cases in for printf processing (allocations in argument list and long double processing, return value exceeding INT_MAX), but they don't seem to apply here.
There was a problem hiding this comment.
@fweimer-rh might be extremely unlikely, but things may break elsewhere and it is legit for snprintf to return negative values according to documentation.
To be honest, I don't know. Is there some good practice about how paranoid and double-checking is it worth to be (vs. not complicating the code)?
There was a problem hiding this comment.
@fweimer-rh I would like to hear and follow your opinion. Shall we be more paranoid and expect that snprintf might return negative values as documented, or less paranoid and rely on no libc implementation failing for some specific arguments?
| "$%c$rounds=%lu$", tag, count); | ||
| { | ||
| int w = snprintf ((char *)output, output_size, | ||
| "$%c$rounds=%lu$", tag, count); |
There was a problem hiding this comment.
Likewise I believe this invocation cannot fail.
|
@fweimer-rh @besser82 I am sensing reluctance to this change. Would it be easier for everybody to just close it and never look back? |
lib/crypt-pbkdf1-sha1.c
Outdated
| @@ -148,13 +148,15 @@ crypt_sha1crypt_rn (const char *phrase, size_t phr_size, | |||
| } | |||
|
|
|||
| sl = (size_t)(sp - setting); | |||
| assert (sl <= CRYPT_SHA1_SALT_LENGTH); | |||
There was a problem hiding this comment.
I wonder if this assert can be hit by crafted inputs, then this change might be problematic. Is there a way to return an error for this?
The remaining asserts look fine.
In my previous comments, I probably got carried away by our internal analysis tool discussion, sorry.
There was a problem hiding this comment.
@fweimer-rh You are right, this assert could be hit if long enough input is provided. I'll remove it. The limit CRYPT_SHA1_SALT_LENGTH is actually used only when generating the salt and hashing works fine with longer salts UNTIL CRYPT_OUTPUT_SIZE (384) starts to be a problem - in that case only the salt is present in crypt_sha1crypt_rn. This is an issue in my opinion, but probably for another PR.
I'll remove this assert.
There was a problem hiding this comment.
@fweimer-rh could you have a look now?
snprintf returns negative values in case of errors, as found out by SAST (Static Application Security Testing)
|
@fweimer-rh could you please have another look if all your concerns are fixed? |
snprintf returns negative values in case of errors, as found out by SAST (Static Application Security Testing)