misc: better random strings

[piru]

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.

[jay]

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

[dfandrich]

Don't bother to spend much time on that. This is definitely micro.

[piru]

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]

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]

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]

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]

Some quick research on the separator browsers use these days:

• Chromium about charset to use: https://github.com/chromium/chromium/blob/466969ca58ae8686107d4a971c7918862158d5bc/net/base/mime_util.cc#L799
• Actual Chromium boundary generation: https://github.com/chromium/chromium/blob/466969ca58ae8686107d4a971c7918862158d5bc/net/base/mime_util.cc#L874
• It seems Mozilla uses 3 x random numbers so bunch of digits:
  https://github.com/mozilla/gecko-dev/blob/1cae9ca7cc7d17cfc92088dfc68e5ff391128caa/dom/html/HTMLFormSubmission.cpp#L355

[dfandrich]

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]

[bagder]

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?

[dfandrich]

The PR is not quite ready to submit? ☺

[bagder]

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]

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

[bagder]

Fixed