On Tue, Jan 31, 2012 at 10:23 AM, Erich E. Hoover ehoover@mines.edu wrote:
On Tue, Jan 31, 2012 at 10:04 AM, Juan Lang juan.lang@gmail.com wrote:
Sorry I didn't spot this earlier. Without this, someone who registers a certificate common name with an embedded NULL, like "codeweavers.com\0.badguy", could fool crypt32 into accepting it for a domain it isn't registered to, codeweavers.com in my example.
It looks like you've just changed it to allow more than one NULL at the end... It seems to me that the matching code already handles the case of an embedded NULL, since it goes through the allowed_len characters and manually checks each byte (rather than using a routine like strcmp() which stops at NULLs).
Please forgive the "Reply to all" fail.
Erich Hoover ehoover@mines.edu
Hi Erich,
On Tue, Jan 31, 2012 at 9:34 AM, Erich E. Hoover ehoover@mines.edu wrote:
On Tue, Jan 31, 2012 at 10:23 AM, Erich E. Hoover ehoover@mines.edu wrote:
On Tue, Jan 31, 2012 at 10:04 AM, Juan Lang juan.lang@gmail.com wrote:
Sorry I didn't spot this earlier. Without this, someone who registers a certificate common name with an embedded NULL, like "codeweavers.com\0.badguy", could fool crypt32 into accepting it for a domain it isn't registered to, codeweavers.com in my example.
It looks like you've just changed it to allow more than one NULL at the end... It seems to me that the matching code already handles the case of an embedded NULL, since it goes through the allowed_len characters and manually checks each byte (rather than using a routine like strcmp() which stops at NULLs).
Well, sort of. The byte-by-byte comparison takes place component by component. The boundary between each component is defined by the presence of a '.'. That's why, in my example, I have an embedded NULL immediately prior to a '.'. After the end of each component is found, it's passed to match_domain_component. In the current git version, each component strips a NULL, hence an embedded NULL is accepted. In the version I sent, only trailing NULL(s) are removed.
You're right that I allow multiple trailing NULLs rather than just one, but that difference seems immaterial. The key one is to prevent NULLs immediately prior to dots. --Juan
On Tue, Jan 31, 2012 at 11:10 AM, Juan Lang juan.lang@gmail.com wrote:
... Well, sort of. The byte-by-byte comparison takes place component by component. The boundary between each component is defined by the presence of a '.'. That's why, in my example, I have an embedded NULL immediately prior to a '.'. After the end of each component is found, it's passed to match_domain_component. In the current git version, each component strips a NULL, hence an embedded NULL is accepted. In the version I sent, only trailing NULL(s) are removed. ...
Wow, I clearly didn't read that you moved the code over to match_common_name. Apparently I'm not quite conscious today, my apologies!
Erich Hoover ehoover@mines.edu
Wow, I clearly didn't read that you moved the code over to match_common_name. Apparently I'm not quite conscious today, my apologies!
Clearly I wasn't quite conscious when I reviewed your patch in the first place :) No worries, for security fixes lots of checking is always worthwhile! --Juan
-
Erich E. Hoover -
Juan Lang