
Security advisory: Insufficient data validation in yubikey-val - QUFB
https://www.yubico.com/support/security-advisories/ysa-2020-01/
======
tptacek
This appears to be the open-source PHP project (an alternative to Yubikey's
cloud service) that validates OTPs. Most people who use Yubikeys don't use the
OTP feature at all; I don't know how many of them use the PHP yubikey-val
service.

Not that it's not important, but I assume this story's placement on the front
page is a consequence of people thinking this impacts Yubikeys themselves
through U2F (the way most people use keys) or SSH/PGP (the way most of the
rest of people do).

Here's the commit that fixes the bug:

[https://github.com/Yubico/yubikey-
val/pull/59/commits/d0e4db...](https://github.com/Yubico/yubikey-
val/pull/59/commits/d0e4db3245deb5ce0c8d7d26069c78071a140286)

It's... not great; it looks like they were accepting SQL metacharacters in
hand-rolled non-parameterized SQL queries.

If you were running this project: (1) you should fix right away and presumably
disregard the summary at the top of the advisory that this vulnerability would
just allow DoS, and (2) once you do, tell the rest of us why you were running
this thing; I'm sure we'd be interested in hearing your use case.

~~~
sombremesa
How could someone working in a security context in modern times be using non-
parameterized queries? I know the answer is along the lines of "because PHP"
but I just can't believe it. This really makes me paranoid regarding other
security SaaS.

~~~
tptacek
It's an old open source project that isn't really actively developed. But
also: I'd resist the inclination to believe that security product companies
are especially good at building secure software.

~~~
carterehsmith
>> It's an old open source project that isn't really actively developed.

Is it actively used?

------
jagged-chisel
> ... insufficient data validation in the open-source project for YubiKey
> Validation Server

This brought clarity to the headline for me. Hope it helps others before
clicking.

~~~
anon102010
Much better - and default config is a denial of service impact issue (not
remote code or replay).

~~~
tptacek
I'm not sure I trust the "it's SQL injection but you can only use it to stuff
bad records into the database" summary here. SQL is usually an easy thing to
pivot from.

~~~
dasyatidprime
The way I read the advisory is: (1) The most harmful vulnerability represented
anywhere in this advisory is SQL injection. (2) The sync endpoint allows a
full SQL injection, but the sync endpoint is normally IP-restricted and has an
empty allow list in the default configuration, which can mitigate the
exposure. (3) The verify endpoint does not allow a full SQL injection, but
allows overly long values to be passed through to SQL.

The first paragraph in “Technical details” mixes (1) and (3) confusingly, and
should have had a paragraph break before the sentence beginning “Verify” (or
been rewritten).

A quick glance at the patchset suggests this is right (the verification code
looks to have been made stricter in the verify endpoint, but _added_ in the
sync endpoint), but I didn't dig deeper than that.

[edited slightly for wording]

~~~
tptacek
Thanks! This is super helpful.

------
notlukesky
Explained quite clearly.

