Security advisory: Insufficient data validation in yubikey-val | Hacker News Reader

Security advisory: Insufficient data validation in yubikey-val

yubico.com

85 pointsQUFB18 comments

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...

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.

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.

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.

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

Is it actively used?

"Because PHP" isn't the answer. PHP has had parameterized queries forever, and modern PHP looks little like the patch linked. :(

For some version of forever that, to my recollection, doesn't include working with mysql 4.0. (Which was a long time ago, but still, Perl has had parameterized queries since forever via DBI, even on databases that didn't supprt them).

"because PHP" is absolutely not the answer. I haven't touched PHP since 5.5, but even since then, PHP had a very solid SQL interface (mysqli and most importantly, PDO), which offer a very beautiful and fully functional interface to parametrized queries à la JDBC.

And even then, the PHP documentation was ALWAYS telling people "PLEASE FOR THE LOVE OF EVERYTHING THAT IS HOLY MOVE TO PDO AND STOP USING NON-PARAMETRISED APIs"

But the problem is that with PHP, a LOT of the problems are usually resolved by programmers using the age old "copy pasta from a stackoverflow thread that's 10 years old".

However, I do concede that for someone who's in a security context, it is absolutely narrow minded to allow ANYTHING beyond parametrized queries.

Because outside of the bubble of the top 1% of the top 1% of security teams actual software engineering knowledge is barely existent

No parametrized PHP driver for Postgres allows pipelining queries. This is important if your talking to something non-vanilla-PG that has internal pipelining abilities to mostly elide internal latency from the transaction duration.

I use it to secure a web app. I’ve customized the instance so that when the validation succeeds, it returns an encryption key used to encrypt the data in the database. The idea was to keep the encryption key off of the app server save for mem in session. This key functionality is why I’m using it rather than the Yubico provided service. It’s an internal app not exposed to the net.

It’s not using the sync service and the server is ip locked via iptables.

Thanks for the info.

Interesting, thanks!

We’ve been using this project for a little over ten years as our first attempt to implement MFA across a number of systems.

At the time (ten years ago!) there was concern about trusting cloud providers, so we had pressure to implement it on-prem. Additionally the cloud offering at the time only validated the OTP, but all user to token tracking was left up to the individual system, so we ended up customizing it quite heavily to allow us to automate token provisioning and track user to token mappings, and provide a RADIUS front end so we could tie in other systems.

We’re finally looking to retire it, but it’s had a good long life.

> ... 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.

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

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.

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]

Thanks! This is super helpful.

Explained quite clearly.

Reply on news.ycombinator.com