5 ms·

Security advisory: Insufficient data validation in yubikey-val

85 points by QUFB 7y ago

jagged-chisel 7y ago
> ... 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 7y ago
  Much better - and default config is a denial of service impact issue (not remote code or replay).
  - tptacek 7y ago
    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 7y ago
      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 7y ago
        Thanks! This is super helpful.
notlukesky 7y ago
Explained quite clearly.
tptacek 7y ago
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/d0e4db3245deb5ce0c8d7d26069c78071a140286 https://github.com/Yubico/yubikey-val/pull/59/commits/d0e4db... 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 7y ago
  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 7y ago
    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 7y ago
      >> It's an old open source project that isn't really actively developed. Is it actively used?
  - claar 7y ago
    "Because PHP" isn't the answer. PHP has had parameterized queries forever, and modern PHP looks little like the patch linked. :(

85 points by QUFB 7y ago

> ... 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 7y ago
Much better - and default config is a denial of service impact issue (not remote code or replay).
- tptacek 7y ago
  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 7y ago
    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 7y ago
      Thanks! This is super helpful.

notlukesky 7y ago

Explained quite clearly.

tptacek 7y ago

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/d0e4db3245deb5ce0c8d7d26069c78071a140286 https://github.com/Yubico/yubikey-val/pull/59/commits/d0e4db... 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 7y ago
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 7y ago
  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 7y ago
    >> It's an old open source project that isn't really actively developed. Is it actively used?
- claar 7y ago
  "Because PHP" isn't the answer. PHP has had parameterized queries forever, and modern PHP looks little like the patch linked. :(