Skip to content

hardening: negative snprintf return values #182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ep69 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

hardening: negative snprintf return values #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/crypt-pbkdf1-sha1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ crypt_sha1crypt_rn (const char *phrase, size_t phr_size,
*/
dl = snprintf ((char *)output, out_size, "%.*s%s%lu",
(int)sl, setting, magic, iterations);
assert (dl > 0);
/*
* Then hmac using <phrase> as key, and repeat...
*/
Expand All @@ -165,8 +166,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$",
Copy link

@t184256 t184256 Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's another one above (L156)

Copy link
Contributor Author

@ep69 ep69 Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@fweimer-rh fweimer-rh Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ep69 ep69 Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fweimer-rh thanks for looking, I'll check.

magic, iterations, (int)sl, setting);
assert (dl > 0);
pl = (size_t) dl;
ep = output + pl;

/* Every 3 bytes of hash gives 24 bits which is 4 base64 chars */
Expand Down
1 change: 1 addition & 0 deletions lib/crypt-sha256.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ crypt_sha256crypt_rn (const char *phrase, size_t phr_size,
int n = snprintf (cp,
SHA256_HASH_LENGTH - (sizeof (sha256_salt_prefix) - 1),
"%s%zu$", sha256_rounds_prefix, rounds);
assert (n > 0);
cp += n;
}

Expand Down
1 change: 1 addition & 0 deletions lib/crypt-sha512.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ crypt_sha512crypt_rn (const char *phrase, size_t phr_size,
int n = snprintf (cp,
SHA512_HASH_LENGTH - (sizeof (sha512_salt_prefix) - 1),
"%s%zu$", sha512_rounds_prefix, rounds);
assert (n > 0);
cp += n;
}

Expand Down
5 changes: 3 additions & 2 deletions lib/crypt-sunmd5.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,9 @@ gensalt_sunmd5_rn (unsigned long count,

assert (count != 0);

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);
Copy link
Contributor

@fweimer-rh fweimer-rh Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ep69 ep69 Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)?

Copy link
Contributor Author

@ep69 ep69 Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

assert (written > 0);


write_itoa64_4(output + written + 0, rbytes[2], rbytes[3], rbytes[4]);
Expand Down
8 changes: 6 additions & 2 deletions lib/util-gensalt-sha.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,12 @@ gensalt_sha_rn (char tag, size_t maxsalt, unsigned long defcount,
written = 3;
}
else
written = (size_t) snprintf ((char *)output, output_size,
"$%c$rounds=%lu$", tag, count);
{
int w = snprintf ((char *)output, output_size,
"$%c$rounds=%lu$", tag, count);
Copy link
Contributor

@fweimer-rh fweimer-rh Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise I believe this invocation cannot fail.

assert (w > 0);
written = (size_t) w;
}

/* The length calculation above should ensure that this is always true. */
assert (written + 5 < output_size);
Expand Down