Skip to content
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

Jump to bottom

misc: better random strings #11838

Closed
wants to merge 1 commit into from
Closed

misc: better random strings #11838

wants to merge 1 commit into from

Conversation

piru
Copy link

@piru piru commented Sep 12, 2023

Generate alphanumerical random strings.

Prior this change curl used to create random hex strings. This was mostly okay, but having alphanumerical random strings is better: The strings have more entropy in the same space.

The MIME multipart boundary used to be mere 64-bits of randomness due to being 16 hex chars. With these changes the boundary is 22 alphanumerical chars, or little over 130 bits of randomness.

@github-actions github-actions bot added the tests label Sep 12, 2023
dfandrich
dfandrich reviewed Sep 12, 2023
View reviewed changes
lib/rand.c Outdated Show resolved Hide resolved
@jay
Copy link
Member

jay commented Sep 12, 2023

The restrictions may have been for legacy server compatibility? @monnerat

misc: better random strings
9051dad 
Generate alphanumerical random strings.

Prior this change curl used to create random hex strings. This was
mostly okay, but having alphanumerical random strings is better: The
strings have more entropy in the same space.

The MIME multipart boundary used to be mere 64-bits of randomness due
to being 16 hex chars. With these changes the boundary is 22
alphanumerical chars, or little over 130 bits of randomness.
@dfandrich
Copy link
Contributor

dfandrich commented Sep 12, 2023 via email

@piru
Copy link
Author

piru commented Sep 12, 2023

Regarding the legacy server compatibility aspect: I did not make Digest Auth use alnum, even though cnonce could be made to use it as well. RFC 7616 https://datatracker.ietf.org/doc/html/rfc7616 says The cnonce value is an opaque quoted ASCII-only string. All examples just use hex string there, probably due to symmetry. Due to this I'm sure there might be some broken implementations that would b0rk on actual non-hex cnonce, and thus it likely is wise to avoid using one there.

For other things using alnum should be safe, I think.

@monnerat
Copy link
Contributor

The restrictions may have been for legacy server compatibility?

No, I did not have such limitation in mind when I wrote mime: I used this procedure because it was available and already used for that purpose in the legacy form API code:

curl/lib/formdata.c

Lines 1555 to 1567 in 5bae727

static CURLcode formboundary(struct Curl_easy *data,
char *buffer, size_t buflen)
{
/* 24 dashes and 16 hexadecimal digits makes 64 bit (18446744073709551615)
combinations */
if(buflen < 41)
return CURLE_BAD_FUNCTION_ARGUMENT;
memset(buffer, '-', 24);
Curl_rand_hex(data, (unsigned char *)&buffer[24], 17);
return CURLE_OK;
}

In fact, the randomness is not very important in mime: there is no cryptographic and/or secrecy requirements and the only things we need are:

  • No 2 identical nested boundaries,
  • Not matching a real data line.

Both are already unlikely to happen.

@piru
Copy link
Author

piru commented Sep 12, 2023

No MIME RFC spells it out explicitly, but having very low entropy or even predictable boundary would lead to security impact where the attacker could inject arbitrary multiparts via crafted message contents. This of course wasn't the case here, 64-bit of strong randomness is already very difficult to target in any meaningful way.

@bagder
Copy link
Member

bagder commented Sep 13, 2023

My recollection of the layout of the boundary separator:

It started out without the long sequence of dashes but was modified to look like this early on (20+ years ago) due to someone having problems with the first approach because - as always - users who wrote server side scripts (PHP?) did lots of assumptions about how to parse and detect that boundary based on how the boundaries that were used by the popular browsers at the time used.

We then adapted to this version that looks much more like the browsers' back then.

I don't know if this can still be a problem. I don't know if this change actually makes this not look like a browser separator. I don't know if the browsers perhaps have even modified their separators since then.

@piru
Copy link
Author

piru commented Sep 13, 2023

Some quick research on the separator browsers use these days:

@bagder bagder closed this in 3aa3cc9 Sep 16, 2023
@dfandrich
Copy link
Contributor

This is causing a warning treated as an error on Windows VS2002 on Appveyor:

C:\projects\curl\lib\rand.c(278,19): error C2220: the following warning is treated as an error [C:\projects\curl\lib\libcurl_object.vcxproj]
C:\projects\curl\lib\rand.c(278,19): warning C4295: 'alnum': array is too small to include a terminating null character [C:\projects\curl\lib\libcurl_object.vcxproj]

@dfandrich dfandrich reopened this Sep 16, 2023
@bagder
Copy link
Member

bagder commented Sep 16, 2023

It's better to create a new issue for that, since this PR has already been merged it is unclear what it means to reopen it?

bagder added a commit that referenced this pull request Sep 16, 2023
@bagder
rand: fix 'alnum': array is too small to include a terminating null c…
56719f6 
…haracter

It was that small on purpose, but this change now adds the null byte to
avoid the error.

Follow-up to 3aa3cc9

Reported-by: Dan Fandrich
Ref: #11838
@bagder bagder mentioned this pull request Sep 16, 2023
Closed
@dfandrich
Copy link
Contributor

dfandrich commented Sep 16, 2023 via email

@bagder
Copy link
Member

bagder commented Sep 16, 2023

The PR is not quite ready to submit? ☺

But the commit was not reverted so this PR cannot be merged (again). The code that is merged needs a (different) fix, not this PR... It's not terribly important, I made #11870 to hopefully fix it.

@piru
Copy link
Author

piru commented Sep 16, 2023

Right. Let this right here be a warning for anyone trying to micro optimize. 😂

bagder added a commit that referenced this pull request Sep 17, 2023
@bagder
rand: fix 'alnum': array is too small to include a terminating null c…
706eff9 
…haracter

It was that small on purpose, but this change now adds the null byte to
avoid the error.

Follow-up to 3aa3cc9

Reported-by: Dan Fandrich
Ref: #11838
Closes #11870
@bagder
Copy link
Member

bagder commented Sep 17, 2023

Fixed

@bagder bagder closed this Sep 17, 2023
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
@ptitSeb
misc: better random strings
0223844 
Generate alphanumerical random strings.

Prior this change curl used to create random hex strings. This was
mostly okay, but having alphanumerical random strings is better: The
strings have more entropy in the same space.

The MIME multipart boundary used to be mere 64-bits of randomness due
to being 16 hex chars. With these changes the boundary is 22
alphanumerical chars, or little over 130 bits of randomness.

Closes curl#11838
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
@bagder @ptitSeb
rand: fix 'alnum': array is too small to include a terminating null c…
dd82348 
…haracter

It was that small on purpose, but this change now adds the null byte to
avoid the error.

Follow-up to 3aa3cc9

Reported-by: Dan Fandrich
Ref: curl#11838
Closes curl#11870
@jcamiel jcamiel mentioned this pull request Oct 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfandrich dfandrich dfandrich left review comments

Assignees
No one assigned
Labels
tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@piru @jay @dfandrich @monnerat @bagder