Created attachment 29882 [details] Patch to replace MD5 with an EVP implementation of SHA1 OpenSSL 1.0.1c strongly deprecated direct access to low level MD5/SHA hash functions when running in FIPS mode, in favor of the EVP API. tcnative cannot toggle FIPS mode when combined with this version, and must be ported to the higher-level EVP digest api. Since it is entirely reasonable to drop MD5 altogether from an OpenSSL build, while I was patching this to use the EVP API for this ***non-cryptographic*** hash application (associating a ssl session key with an explict host:port, in a fixed length key), I made the shift from MD5 to SHA1, as it should be expected to survive for a longer period of time than MD5. Another alternative would be to hash with an even longer key, then fold segments of the hash with an xor into a smaller result size. The only collision to be avoided are each of the virtual host name:port combinations sharing the same session cache. But I saved that as an exercise for another day. It turned out that SSL_vhost_algo_id() was unused; that implementation did not even match the implementation represented by the in-line code, and seemed more productive to simply drop this stub entirely.
Before you need to dig too deeply, mod_ssl solved this by adopting apr-util's md5 hash instead of changing over to OpenSSL EVP. tcnative doesn't yet consume apr-util and I saw no reason to add complexity to the build.
In ssl_network.c:154, I'm not sure you wanted this: sizeof(ctx->context_id) The sizeof will likely return the word size on the machine instead of the maximum number of characters ctx->context_id can contain. I think the proper diff for that file is this: - MD5_DIGEST_LENGTH); + SHA_DIGEST_LENGTH); Without this patch, what is the behavior? Does OpenSSL explode when you try to put it into FIPS mode? With this patch, does everything seem to work?
You are suggesting C (C++) can morph the sizeof(str1) member when it is explicitly declared? I think not... struct { char str1[5]; int inta[4]; } any more than it would be allowed to morph the sizeof(str1) here; union { char str1[5]; int inta[4]; ulong uv; } You are confusing the sizeof operator with explicitly computing member offsets; ssize_t mbroffset = ((char*) x.inta[0] - (char*) x.str1[0]) which would, as you pointed out in comment #2, return something more like '8' such that inta remains int-aligned. In any case, the reason for performing something like memcpy(x, sizeof(x)... is that construct isn't subject to being patched again. The context_id member can be changed again and again without mauling that code anymore. I prefer the patch as submitted, for clarity.
Straightforward server.xml config; <Listener SSLEngine="on" FIPSMode="on" className="org.apache.catalina.core.AprLifecycleListener"/> Since 1.0.1c Catalina error log now reports md5_dgst.c(74): OpenSSL internal error, assertion failed: Low level API call to digest MD5 forbidden in FIPS mode! and proceeds to exit before initialization can be completed. From OpenSSL 1.0.1c changelog; *) Low level digest APIs are not approved in FIPS mode: any attempt to use these will cause a fatal error. Applications that really want to use them can use the private_* version instead. [Steve Henson] and from OpenSSL docs since antiquity, warning that this would happen someday; "Applications should use the higher level functions EVP_DigestInit(3) etc. instead of calling the hash functions directly."
Q. With this patch, does everything seem to work? A. Better still, it actual does successfully initialize the listeners and correctly handle incoming SSL traffic, using the SHA1 key into the SSL session cache for session reuse. Thanks for the initial patch review! I believe I still have commit and would be happy to commit this patch myself but would appreciate a couple of eyeballs, particularly Mladen's review as he wrote the function in svn r300723 that this patch now does away with.
(In reply to comment #3) > You are suggesting C (C++) can morph the sizeof(str1) member when > it is explicitly declared? I think not... I wasn't suggesting that. I was suggesting that sizeof(member) isn't what you want when you really want to pass the maximum size of the buffer. Your code only works because 8 (the number returned on my system when I call sizeof(unsigned char *)) is less than the actual size of the buffer which is actually SHA_DIGEST_LENGTH (decimal 20). > You are confusing the sizeof operator with explicitly computing member > offsets; No, I'm not. You are confusing sizeof with strlen, which might cause a buffer overrun. > In any case, the reason for performing something like memcpy(x, sizeof(x)... > is that construct isn't subject to being patched again. The context_id > member > can be changed again and again without mauling that code anymore. I prefer > the > patch as submitted, for clarity. Please look again. This is in the call to SSL_set_session_id_context which accepts a pointer to the session id and a number of bytes that can be read. Your number of bytes is wrong: it is too short.
I think you are confused? ctx->context_id is not a pointer but a byte array. See the declaration; unsigned char context_id[SHA_DIGEST_LENGTH]; context_idis a member of the struct, *context_id is not a member of the struct. sizeof(ctx->context_id) is in fact 20. sizeof(&ctx->context_id) would in fact be 8 as you observe.
(In reply to comment #7) > I think you are confused? ctx->context_id is not a pointer but a byte array. Er... > See the declaration; > > unsigned char context_id[SHA_DIGEST_LENGTH]; > > [...] > > sizeof(ctx->context_id) is in fact 20. > > sizeof(&ctx->context_id) would in fact be 8 as you observe. Aah, I forgot that sizeof (specifically) can resolve that as a statically-sized array instead of returning the size of the pointer type. context_id is most definitely a pointer to "unsigned char" no matter what you say. sizeof is an aberration in this respect (and documented as such in Harrison & Steele). I still think you should use either SHA_DIGEST_LENGTH or even define yet another constant like TCN_DIGEST_LENGTH to be an alias of SHA_DIGEST_LENGTH. Using sizeof is misleading at best.
(In reply to comment #8) > (In reply to comment #7) > > I think you are confused? ctx->context_id is not a pointer but a byte array. > context_id is most definitely a pointer to "unsigned char" no matter what > you say. Sorry, that's simply not true. Although the C language automatically casts an array reference to a pointer where appropriate (and sometimes when not), they are semantically different. > sizeof is an aberration in this respect (and documented as such in > Harrison & Steele). It is clearly not an aberration, but rather appropriate use of the sizeof operator (it is _not_ a function). The only mistake I find in the patch is the unnecessary and misleading use of parentheses around the operand, which should only be used when the operand is a type, not a field; since sizeof is an operator, the parentheses are used as in a cast, and should not be used otherwise. > I still think you should use either SHA_DIGEST_LENGTH or even define yet > another constant like TCN_DIGEST_LENGTH to be an alias of SHA_DIGEST_LENGTH. > Using sizeof is misleading at best. Again, I disagree; it is best to use sizeof referencing the target field here since if the field size changes to use a different constant, one does not need to hunt down the all the uses and change them; the revised size is picked up automatically by the sizeof operator. - Chuck
Agreed with comment #9, the patch should be adjusted from sizeof(ctx->context_id) to read sizeof ctx->context_id, which might have avoided the 'promote to pointer' confusion in the comment thread above. I would still want mladen's feedback on removing the never-used SSL_vhost_algo_id function he introduced in r300723 - I simply don't know tcnative versioning rules, so to be his review I've added him as an explicit cc. See comment #5 above. If anyone else can offer their thoughts on removing an exported-but-unused function, please share. Thanks.
SSL_vhost_algo_id can be removed since its unused.
Patch applied, thanks.
Thanks for the review Mladen. Users tracking a version number for this bugfix should look at 1.1.26 or later.