Possible off-by-1 in crypt32/chain.c match_common_names

[William Pettersson]

Hey Juan (+list),
Trying to get the new Blizzard systemcheck program working under Wine, I ran
into an SSL issue. Initially the issue looked like:

trace:chain:match_dns_to_subject_dn L"sea.battle.net"
trace:crypt:CryptDecodeObjectEx (0x00000001, #0014, 0x14f5bc, 115,
0x00008001, (nil), 0xd1c25c, 0xd1c258)
trace:crypt:CryptDecodeObjectEx returning 1
trace:crypt:CertFindRDNAttr "0.9.2342.19200300.100.1.25" 0x153fc8
trace:chain:match_common_name CN = L"*.battle.net\0000"
warn:chain:match_domain_component domain component L"net" too short for
L"net\0000"
trace:chain:match_common_name returning 0

and resultingly I would get

IRC helped point out that the "\0000" seems to be an off-by-one error in the
length of a string. It seems, in match_common_name(), that allowed_len
currently denotes the length of a string including the terminating NUL, but
server_len doesn't include the terminating NUL. This results in comparisons
not playing nice.

I've tested simply by adding "allowed_len--;" and this does mean that my app
now can successfully negotiate an SSL connection. I'm not really up on
crypto in Wine, so I'm not sure if this actually is an off-by-one error, and
if so where it should be fixed.

I do think that nameAttr->Value.cbData should be used, as technically I
think \0 characters in URLs are now allowed, so strlenW would not work
correctly (and would be a security issue?), but then it seems the best fix
is simply to just decrement allowed_len.

Any thoughts or ideas on whether this is actually a bug and if so, how to
fix it?

William

[Juan Lang]

Hi William,
> trace:chain:match_common_name CN = L"*.battle.net\0000"
> warn:chain:match_domain_component domain component L"net" too short for
> L"net\0000"

That CN is coming from the certificate.
> Any thoughts or ideas on whether this is actually a bug and if so, how to
> fix it?

It's partly a bug in the certificate.  x.509 states that strings do
not include a terminating NULL.  Some providers erroneously include
one anyway.  Whether it's a bug in Wine depends on what Microsoft
does.  It's not an off-by-one bug, though:  the code is working as
intended, for the moment.

As a workaround, one might check all names twice:  once without the
terminating NULL (preferred), and once with it.  You wouldn't want to
use strlenW on the names in the certificates.  Actually doing so
throughout the code will be hard, unfortunately.
--Juan