Not validating source of callback

[ha1331]

Not validating source of callback

[scholtzm]

@mnzt Since OpenID spec allows this ("supply any OpenID provider"), I don't think this will be changed in packages such as node-openid.

However, I think the fix has already been suggested - just validate the request itself.

A quick fix for anyone running this package right now would be to set options.passReqToCallback and then just validate op_endpoint, e.g.:

// taken from the example
passport.use(new SteamStrategy({
    returnURL: 'http://localhost:3000/auth/steam/return',
    realm: 'http://localhost:3000/',
    apiKey: 'key',
    passReqToCallback: true
  },
  function(req, identifier, profile, done) {
    if(req.query['openid.op_endpoint'] !== 'https://steamcommunity.com/openid/login' ||
       !/^http:\/\/steamcommunity\.com\/openid\/id\/\d+$/.test(identifier)) {
      return done(null, false, { message: 'Claimed identity is invalid.' });
    }

    return done(null, profile);
  }
));

This can be done inside passport-steam as well.

[ha1331]

@KonDax Well if you say so. Tho the reality of me actually copy pasting an url where I only changed the domain/callback location and steam id to be able to login to multiple sites, including the example implementation from this repo as any user that existed and you saying it doesnt work are not in sync. Don't know how valid your claim is, but I know this works, since I have tried it. Also I know for a fact this has been used to steal skins. This has been used to extort actual dollars for a fix.Are you saying they just claimed it works and these site owners just didn't check, just payed for a fix for a problem that doesn't exist and imagined they experienced losses in excess of tens of thousands?

[ha1331]

@welps I tested it, after wiping node_modules and doing npm install I'm happy to say the "exploit" as it was, does not work anymore.

@KonDax I apologize if my responses came out strong, did not know work had been done to fix this, seeing how the issue was still open and all that.

[scholtzm]

@cocothewithlovein Can you add logging of op_endpoints if it's invalid? I'm curious to see what is being used.

I dug into this a bit further:

1. For whatever reason, GetPlayerSummaries WebAPI allows SteamIDs to be passed in in a very random format, e.g. this works https://api.steampowered.com/ISteamUser/GetPlayerSummaries/v2/?key=KEY&format=json&steamids=http://steamcommunity.com.banana.com/openid/id/76561197960435530.
2. This line of code uses the whole identifier, but should only provide the SteamID itself.
3. Received identifier should be validated with RegEx (@KonDax' previous PR was right about this, my bad 👍 ) to make sure it has the following format: http://steamcommunity.com/openid/id/{7656\d+$}

TL;DR Final patch should validate identifier format and ideally op_endpoint as well. I'm thinking this would have never happened if GetPlayerSummaries only accepted actual SteamIDs, since then the identifier would have to be parsed in the first place.

[welps]

Fixed in #41 by @scholtzm

Thanks everyone!

[tobbbles]

You raise a very good point, there is quite a high potential for mitm attack for openid and steam.

I don't see any reason the package cannot verify the source on the return payload - I'll see what I can muster up after work

[breatoz]

Can you delete this post for now, we're working on patching it and don't want others exploiting sites.

Thx.

[tobbbles]

@breatoz No, we will not delete this. I am working on a patch which ought to clear this up shortly

[welps]

@mnzt Somewhat related to this is #24 -- we're blocked by upstream still utilizing node-openid 1.x.x. I've been meaning to open a PR to investigate and get passport-openid to using node-openid 2.x.x but literally have had no time since starting new job.

[tobbbles]

@welps I've had to open up my own fork to fix (#34) to fix this (https://github.com/spaceshuttl/passport-openid/commit/60daa5281eb180c19c92ae783d836e3070bf523b).

[welps]

@mnzt oh, I see. I could've sworn that node-openid 2.x.x has breaking changes that passport-openid has to be updated for. Is that not the case?

[tobbbles]

@welps Not from what I can see, I tested the example running on node-openid 2.x.x and it all seemed fine

[tobbbles]

@ha1331 just to note, as @breatoz posted before deleting, you can temporarily verify via

if (req.query['openid.claimed_id'].substr(0,26) != "http://steamcommunity.com/"
|| req.query['openid.identity'].substr(0,26) != "http://steamcommunity.com/"
|| req.query['openid.op_endpoint'].substr(0,27) != "https://steamcommunity.com/") {
res.redirect('/auth/steam');
return false;
}

You may want to be aware of SSL too.

[tayler-king]

I don't believe this is a valid issue.

I've been attempting many ways to bypass what you seem as no validation in the identified, yet all have failed.

Node-OpenID checks if op_endpoint from the assertion URL is the same as the endpoint from the provider URL, and if it isn't then it fails.

Without this, you cannot change the other values of the domain. Without the secret, you are not able to generate a valid signature. Without a signature that matches the parameters, the verification call fails.

Unless you can find the secret before verifying the assertion URL, you can't modify any IDs or other parameters. This would, like you said, be an issue in Node-OpenID and not passport-steam since the secret is never released.

[ha1331]

@KonDax I believe this is as valid issue as issue can be. Seeing how multiple sites have been extorted to pay for a fix, bug bounties has been payed in the tens of thousands and this "bug" or rather limiter or not as secure as possible default implementation has been used to make tens of thousands by logging in as any user you know the steam id for.

I really have difficulties understanding how you would think it's not an issue anyone can just "pretend to be steam", point a browser to your callback url and tell it to verify the identity from iamsteam.exploiters.com. Can you present any scenario where steam passport strategy would need to verify the users identity from other places than steam?

[tayler-king]

@ha1331 As I said, OpenID already checks if the callback url matches the strategy's domain, hence why I said it isn't valid.

[tayler-king]

It's strange, since I tried the same as you without success. I attempted to change the op_endpoint to a fake site that returns the is valid response, yet OpenID checks if the domain is the same. I'm unsure of how you achieved it.

Sorry, my mistake.

[ha1331]

We are approaching the limit of my understanding of how openid works, but the way "the exploit" works (I think) is, fakesteam.exploit.com verifies that the user is valid user for fakesteam.exploit.com and that seems to be enough. Problem here is allowing these verifications to other domains than steamcommunity.com. Does that make sense? If something has been patched after I wrote this issue, then I'm happy to say this is not an issue. But I can assure you this worked when I wrote the issue report. I could provide you the url, but it was given to me in confidence, so I cant.

[welps]

@ha1331 do you mind trying to reproduce this on the latest version of passport-steam?

I believe @mnzt updated this package to use a fork of passport-openid where its dependency, node-openid, has been updated to 2.x.x. That version of node-openid addresses the concern brought up in #24

[tayler-king]

@ha1331 then I believe it has been addressed, since with the latest version of passport-steam the exploit seems not to be present.

[welps]

@ha1331 Great to hear. Thanks for reporting the issue.

@mnzt Thanks for the update!

Yeah, sorry for the miscommunication guys. We'll try to keep these tickets up-to-date.

[tobbbles]

Great work on the 🐛 squishing, on behalf of the users of this package, I highly appreciate it

[bartlomiej-korpus]

This bug is NOT fixed. Using [email protected] and [email protected] I can successfully trick example app (examples/signon) to think I am somebody else.

[tobbbles]

Can you provide evidence and a steps to reproduce? In am email would be
preferable (you can find my address on my profile)

On Wed, 8 Jun 2016, 20:20 bartlomiej-korpus, [email protected]
wrote:

This bug is NOT fixed. Using [email protected] and [email protected] I can
successfully trick example app (examples/signon) to think I am somebody
else.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AJoYe2lhskqapM9-GQgGfKa7VyJvvzmrks5qJxYHgaJpZM4IZzYH
.

[welps]

cc me too please

[bartlomiej-korpus]

Confirmed the issue with @mnzt .

[tayler-king]

Is this using the same 'exploit', or is there another way around the new prevention checks?

[tobbbles]

Hi, I've been doing some testing with @bartlomiej-korpus and I can confirm this is a vulnerable issue.

Unfortunately: this permits to the openid spec, so we're having to come up with our own fix, thankfully @bartlomiej-korpus is willing to devote his time into a fix.
Fortunately: It's not so easy to do, however it is quite trivial.

[bartlomiej-korpus]

@KonDax it is the same, it was never fixed in the first place.

[tayler-king]

Gotcha. If you need any help, I'm available.
On Wed, 8 Jun 2016 at 10:27 pm, bartlomiej-korpus [email protected]
wrote:

@KonDax https://github.com/kondax it is the same, in was never fixed in
the first place.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AQFo-hPZIqf6kxxo9FtQKwXpfmPc33Vwks5qJzO9gaJpZM4IZzYH
.

[andrewda]

I can definitely help out, too, and I'll start doing some testing on the best way to avoid this issue. This certainly needs to be fixed ASAP.

[bartlomiej-korpus]

I think one of the ways is to test the claimedIdentifier after successfull assertion against regex like ^http:\/\/steamcommunity.com\/openid\/id\/\d+$
What do you guys think?

[welps]

@bartlomiej-korpus I didn't get to observe the exploit, but if you exploited the way I think you did, then @mnzt 's comment for a fix works too, no? #35 (comment)

[breatoz]

Yes; just double check that I had those variable names right; and its assertive ;p

Sorry I've been swamped and haven't participated more

Sent from my iPhone

On Jun 8, 2016, at 9:05 PM, WC [email protected] wrote:

@bartlomiej-korpus I didn't get to observe the exploit, but if you exploited the way I think you did, then @mnzt 's comment for a fix works too, no? #35 (comment)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bartlomiej-korpus]

Probably yes, but thats not the most elegant way to protect

[tobbbles]

@welps our main concern with that is the ability to use a subdomain, for example steamcommunity.com.github.com, allong with the usage of nullbytes and other such exploits

[bartlomiej-korpus]

I have mailed node-openid author, person experienced with openid protocol to consult the fix with regexes, hope he agrees that it is enough.

[welps]

@mnzt gotcha, seems like regex should work out too then

[tayler-king]

I can get the 'exploit' to work with the latest version on my local server (changing all addresses to http://steamcommunity.com.<xxx>:3000), but I am not able to reproduce the error on an older version on my development server.

[tobbbles]

@KonDax can you please provide version numbers

[tayler-king]

@mnzt Hang on, I'll grab them in a bit. I'll edit this with them.

Edit: just checked, seems as though both are on @v1.0.2. Sorry for the mistake, I must've not accounted for the outside servers being able to communicate with the fake local OpenID provider. I'll run some more tests if you want once I've got that sorted.

[FreddyFish]

Any news on this?

[waylaidwanderer]

I had a discussion with one of the exploiters a few weeks ago and he told me that my site which uses https://github.com/invisnik/laravel-steam-auth is not vulnerable. Although it is PHP, perhaps you can take a look at their validate() implementation.

[tayler-king]

@waylaidwanderer

That library isn't vulnerable since it doesn't call the URL found in the OpenID return parameters but rather a static steamcommunity URL with the OpenID parameters plugged in.

Using the regex @bartlomiej-korpus provided would be enough, but it's also possible to implement a solution that laravel-steam-auth uses.

[waylaidwanderer]

it doesn't call the URL found in the OpenID return parameters but rather a static steamcommunity URL with the OpenID parameters plugged in

Yep, I'm aware. I think this would be the safest solution rather than using a regex, but as you said, both methods would work, although I think the way laravel-steam-auth does it is safer and more straightforward.

[tayler-king]

I think it would be best to call the static URL, but without being hacky and modifying the returned parameters, we couldn't do that.

Edit: After looking, you can't even do that - that's only for the profile mode. This would be useless for the non-profile mode, so checking via regex is probably just enough for that. The only thing I could think of is pushing a pull request to steam-openid to add an option of what endpoint to check.

[scholtzm]

@KonDax has already pointed this out kinda. The Laravel package is safe because it's always using http://steamcommunity.com/openid/login to verify the signature. Same with many other Steam specific packages.

However, node-openid will use query parameter (if provided) to verify the signature: https://github.com/havard/node-openid/blob/master/openid.js#L1243

passport-steam only receives claimed_id (http://steamcommunity.com/openid/id/7656...) and this can be already spoofed any way you like.

TL;DR Use only http://steamcommunity.com/openid/login to verify the signature, nothing else.

[tayler-king]

@scholtzm if we bundled our own node-openid, we could fix the issue. I think this might be the only way to go forwards with it.

[tobbbles]

@KonDax I think that should be our very very last option

[tayler-king]

@mnzt Exactly. The only other option I see is creating a pull request on node-openid to allow specific validation URLs.

[cocothewithlovein]

Oh god so this is what has been giving me nightmares for the past week, I've had rogue logins on my admin account which stole my sleep and was up for 3 days implementing multiple authentication checks, (express cid, jwt, ip checks, socket auth token) I'm glad the fault isn't in my code

[tobbbles]

@cocothewithlovein You should never have admin access reliant on purely OAuth, you should always ensure 2FA on important applications.

We're currently stuck at a crossroad of keeping the dependency structure, or fixing underlying packages

[tayler-king]

@scholtzm That looks like the best solution for now, yet we can't rely on everybody to implement that.

[tayler-king]

I've created a pull request that adds @scholtzm's patch to the example. With the amount of people directly copying the example, it's probably considered better so that the deployed sites are patched from this exploit.

[FreddyFish]

So the fix provided by @scholtzm would fix the exploit?

[tayler-king]

@Pepsai it should be a temporary fix, but yes, it would.

[cocothewithlovein]

@mnzt right, all the top level admin controls are done through a separate server so not any damages besides someone betting $3 through my account.

@scholtzm Thanks for that quick fix, now I can remove the 30 jwt's and ip checks that I had

[welps]

@scholtzm Appreciate all the help you've been contributing, just wanted to say thanks!

Mm that goes for all of you too! 😄

[tayler-king]

@scholtzm I thought my implementation was correct, you put me off haha. I'll update my latest PR to include the identifier checks and use that also.

Updated the PR to validate op_endpoint and identifier. See #40

[scholtzm]

@mnzt @welps This exploit should get more attention. 😃

[FreddyFish]

@scholtzm what do you mean by that?

[breatoz]

I know right; friggin scary

Sent from my iPhone

On Jun 20, 2016, at 11:09 AM, Michael Scholtz [email protected] wrote:

@mnzt @welps This exploit should get more attention. 😃

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[welps]

@KonDax @scholtzm @breatoz Sorry! Is #40 the fix I need to look at?

[tayler-king]

I'd suggest going with the fix mentioned above. No point with my commit as
there isn't a viable solution to fixing the issue.

Would be best to just have the check within the callback - we can't do
anything else.
On Tue, 21 Jun 2016 at 5:42 pm, WC [email protected] wrote:

@KonDax https://github.com/kondax @scholtzm
https://github.com/scholtzm @breatoz https://github.com/breatoz
Sorry! Is #40 #40 the
fix I need to look at?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AQFo-klOftESZihJOTc5bzpGexcF1JZnks5qOBRkgaJpZM4IZzYH
.

[scholtzm]

@welps Yeah that's the one, but it's not done properly.

@KonDax It can (and it should) be done within passport-steam. I can open new PR tomorrow if someone else doesn't do it first.

[tayler-king]

Feel free to do so now - but this has been inactive for a while and I think
the maintainers of the repo are just leaving it.
On Tue, 21 Jun 2016 at 7:09 pm, Michael Scholtz [email protected]
wrote:

@welps https://github.com/welps Yeah that's the one, but it's not done
properly.

@KonDax https://github.com/kondax It can (and it should) be done within
passport-steam. I can open new PR tomorrow if someone else doesn't do it
first.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AQFo-osEz7DfXMYEbH4x5-dIKcMOUvqoks5qOCi5gaJpZM4IZzYH
.

[welps]

@scholtzm Yeah, please open a PR, we'll review, and then merge it in. Thanks.

[tayler-king]

Sounds great.