A bug related to handling of authenticated sessions | Hacker News Reader

A bug related to handling of authenticated sessions

github.blog

215 pointstodsacerdoti47 comments

I had a mini panic attack when I refreshed a waiting PR and was suddenly logged out. I take all the suggested precautions with my GitHub account, so while my mind did wander to the worst case, I figured it was just a weird cookie interaction on my machine.

If anyone at GitHub is reading this, it would have been nice to see an alert with this info on GitHub's status page, which is the first place I checked to see if it was a known issue.

I had the same feeling, I clicked on a tab and was logged out. Immediately thought someone had logged me out.

It's possible (but admittedly it costs developer time) to trigger a logout with reason, which is displayed on e.g. the login screen when users are redirected to it.

Got the login page this morning and immediately wondered if someone was trying to reset my password

I’m very interested in the post mortem about the specific race condition.

Since the article mentions session ids returned to the wrong user my first guesses would be

1) something with thread-local storage saving the the auth info in the request handler, and then coroutines or async code that is overwriting that thread-local with info from a new request before the old request has finished.

2) messagequeue based micro service communication that was not properly checking if a received response belongs to the original request and thus returning mismatched data

Some user request marked cachable when it should not have been...

>1) something with thread-local storage saving the the auth info in the request handler, and then coroutines or async code that is overwriting that thread-local with info from a new request before the old request has finished.

AFAIK github is written in ruby. Does it even have thread local storage?

Yes, at least in Rails you see this a bit:

> Thread.current[:key] = value

Typically you’d only have one request at a time on each thread though. So your examples feel unlikely. Looking forward to finding out what happened here.

> 1) something with thread-local storage saving the the auth info in the request handler, and then coroutines or async code that is overwriting that thread-local with info from a new request before the old request has finished.

This would be my guess. This sounds like Ruby. But I've seen very similar bugs in Node, where user data is leaked across requests. The issue is typically the developer is using a global variable when they should be using the request context/storage.

In Node there is an insidious brother of this bug as well. It's where the developer is initializing some bit of code (often a 3rd party library, or loading a file) and they are doing it per request when they should be doing it once globally at process startup. It's just about the opposite of the first bug I mentioned. The result is Node leaks memory on every request until it runs out of memory and the process dies. This bug can be an incredible pain in the ass to hunt down.

I read a tweet saying: if you want to test whether your service has real engagement, log out everybody and check how many of them log back in. This can be seen as a practical test of it.

Is that why gitlab.com always logs me out?

Apple's App Store Connect do this everyday lol

aws does this every 24 hours for a normal setup and that's pita. They just don't care what their users are doing. Yes I was on middle of something and suddenly saw that I had to log into their web console again. w???

For our service I chose to end sessions at 4 AM local time, but have them last at least 8 hours.

This means:

* sessions last between 8 and 24+8 hours

* if you log in before 20:00, your session ends that night, if you log in after 20:00 it'll last one more day

* since this is a B2B application users are unlikely to use the application during session expiry

* it's possible to calculate the expiry time during log in and it doesn't require tracking session activity

This approach should have similar security properties as a 24h logout, while minimizing the disruption users experience.

you just have to keep thundering herd of logins in mind though.

While the application components can handle the load, sometimes the authentication system may not do well when everyone tries to login at the same time

Slack learnt that the hard way recently

One assume, since this was days in the making, that the GitHub security team had operations spool up extra auth tier capacity before booting all sessions.

For low-friction-to-change services that can be a death-sentence, though. If I have to log in when opening my electric scooter rental app, I close it and open another one and probably won't use the other one until strictly necessary.

My company has the same kind of "problem". A logged out customer might be a lost one. So we try extremely hard to never have to log out people. So when rewriting the app, the store couldn't be touched. When changing authentication provider the old tokens still had to work and be exchanged for a looong time etc.

First time I've seen the homepage in years, neat globe effect!

Plot twist: the whole blog post is fake and they logged out the users just to show them how awesome the globle is

I just noticed it for the first time. And you can spin the globe and mouse over the missile tracks to show what activity is going on. Quite neat.

Strange.. I went to check this out in an incognito tab and it trashed my tab to holy hell!

Edit: Having hardware acceleration disabled doesn't help. :facepalm:

Was very suspicious when I had to log back in in the middle of the day. Definitely would have appreciated an alert banner or notification somewhere. I don't often check the GitHub blog.

While there's not much for GitHub to gain from disclosing the faulty code, I still would've liked to see the actual (pseudo)code that caused the race condition

Yeah, for a code hosting site I’d expect some code examples of what happened so we can all check our code for the same mistakes.

While GitHub's codebase may be a bit more complex than most of the other projects out there, i agree that a writeup of the bug would definitely be useful!

You probably can get the GitHub Enterprise version, "decompile" it and diff the sources

Unlikely.

> The underlying bug existed on GitHub.com for a cumulative period of less than two weeks at various times between February 8, 2021 and March 5, 2021.

> There is no indication that other GitHub.com properties or products were affected by this issue, including GitHub Enterprise Server

Github enterprise does not include commit history, and the above two things together make it sound like this bug was in a change that was being slowly rolled out or a/b tested, and that never made it into a github enterprise release.

It's unlikely anyone outside of github can shed light on this one.

0.001% of sessions actually seems like a lot for Github's scale and such a potentially impactful session leakage, or is my math wrong?

A very small population of accounts in GitHub's words:

> For the very small population of accounts that we know to be affected by this issue, we’ve reached out with additional information and guidance.

If Wikipedia is to be believed, GitHub had 56 million users in September 2020. If we adjust that to 60 million for the time that's passed, that gives us a ballpark of 600 users who were affected.

So no it's not high. But I'm impressed if they were able to accurately quantify that less than 600 users were impacted by this race condition.

But its talking specifically about sessions. There's plenty of inactive users on GitHub as well.

Using 80:20 rules, and assume only 20% of those are active. We could be down to 120 user?

There are also potentially many sessions per user. (Different browsers, even automated tools perhaps)

Sessions != Users. I have two or three active sessions at any point, Multiple browsers + mobile. Since the issue seems to be with cookies, we can probably discard mobile.

Also each time you login/ token refreshes is probably a new session. So they easily have million's of sessions daily. So in the one month period between 6th Feb and 5 March, you have

  **(DAU) * 35* (average sessions / user)** .

So in the ballpark of 10M * 35 * 5 sessions ~ so anywhere between 700-1500 M sessions conservatively . That is in the ballpark of 15,000 users.

Also 0.001% is their belief of the impact. I don't see any evidence or statistical model or details supporting the statement.

I am deeply skeptical of this number without knowing the methodology , as you say it would quite impressive to narrow down a race condition to that level.

Good point, after re-reading I see the 0.001% does apply to sessions rather than users. That should elevate the total users affected, yeah.

It's a race condition, so it had to do with two operations happening in a certain incorrect order. I bet they are able to see the exact situations where it occurred with pretty great certainty by reviewing the logs of the two operations that raced and see where the incorrect operation won. They even say they are invalidating all sessions or of an abundance of caution, indicative that they are pretty sure about it.

You called it! Just another race condition that uses shared objects without protecting them.

>a race condition in a backend request handling process .

I wish they could give more detail on this bit.

They probably want to do a more thorough audit for similar bugs before saying more.

Long ago I found the potential for this kind of race in one of the then very popular app servers. Things to keep you awake at night..

This comes only a few weeks after the author of this post (Mike Hanley) was hired as CSO[1]. Must feel a bit like a trial by fire!

[1] https://github.blog/2021-02-24-hello-from-githubs-new-chief-...

I wonder if https://github.com/advisories/GHSA-cvw2-xj8r-mjf7 is related. It was published overnight too.

From the blog post

> Further, this issue could not be intentionally triggered or directed by a malicious user.

Seems unrelated.

Anyone know if any of the repositories or source code/commits were altered as a result of this?

would be nice to see the git commit.

Well its closed source so thats a shame.

Auth is hard :)

Reply on news.ycombinator.com