Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Deal with any security issues from shared dev apps #2239

Closed
wants to merge 1 commit into from

Conversation

patcon
Copy link
Contributor

@patcon patcon commented Apr 3, 2014

EDIT: Original title: Remove config for shared Bitbucket dev app

Now making this about overall concern, not just Bitbucket


IRC

Bitbucket does not offer scopes, just full access to public and private repos for any app authorized to act on behalf of the user. We should revoke permissions on the app ASAP (if not done), and alert those who may have been effected. Unfortunately, it might be hard to do that if we've already deleted the app, but we should get in touch with BitBucket to find the users who we may have exposed to danger.

I think the worst part is that categorically the people who were at risk are the developers who took the time to spin up gittip and hack on it :(

To Do

  • disclosure post (asap? after we have usernames?)
  • contact bitbucket support for usernames
  • alert affected individuals on bitbucket by email
  • confirm that github never asked for a scope greater than default (ie. "no scope", which is read-only) [https://botbot.me/freenode/gittip/msg/12916152/]
  • confirm that venmo always asked for read-only perms
  • confirm that twitter always asked for read-only scope
  • confirm that bountysource is safe [https://botbot.me/freenode/gittip/msg/12914627/]
  • dig up old convo where we decided this was okay in the first place lost to the sands of time
  • decide how we're going to implement local dev auth going forward

EDIT2:

attack surface = platform * permissions * users

Platform Permission Time Period
Samurai ? Never
Stripe ? Never
Balanced ? Jul 18, 2012 - Apr 3, 2014
Bitbucket public/private read/write (default) Mar 22, 2013 - Apr 3, 2014
Bountysource ? May 23, 2013 - Apr 3, 2014
GitHub (no scope) June 6, 2012 - ?
GitHub user:email ? - Apr 3, 2014
OpenStreetMaps ? Oct 6, 2013 - Apr 3, 2014
Twitter read-only Sept 13, 2012 - Apr 3, 2014
Venmo ? Jan 7, 2014 - Apr 3, 2014

@chadwhitacre
Copy link
Contributor

We've worked with @erikvanzijst in the past on Bitbucket issues. Perhaps he can help us track down the affected users?

@chadwhitacre
Copy link
Contributor

Cf. #822.

@patcon patcon changed the title Remove config for shared BitBucket dev app Deal with any security issues from shared dev apps Apr 3, 2014
@chadwhitacre
Copy link
Contributor

We're using a dev server for both Bountysource and OpenStreetMap.

@chadwhitacre
Copy link
Contributor

Here are the platforms for which we've published secrets:

  1. Samurai—Secret never published.
  2. Stripe—Secret never published.
  3. Balanced—Secret published since e4b0a0f.
  4. Bitbucket—Secret published since 09a9570.
  5. Bountysource—Secret published since dd011de.
  6. GitHub—Secret published 70113d5.
  7. OpenStreetMap—Secret published since 364b78b.
  8. Twitter—Secret published since fc54d00.
  9. Venmo—Secret published since ff9ab95.

@chadwhitacre
Copy link
Contributor

The leak affects anyone who ran Gittip locally and interacted with one of the nine services above using a real personal account on that service.

The leak extends to whatever we were able to do on their behalf on each service.

Therefore, we need to investigate:

  • what we were able to do on each service
  • the period of time we were able to do it
  • the number of real accounts used during that time

I have destroyed some information that would have been helpful:

  • I removed the gittip-dev app on Bitbucket
  • I removed the gittip-dev app on Twitter
  • I revoked all user tokens for gittip-dev on GitHub.

@chadwhitacre
Copy link
Contributor

We only used Samurai for less than a month very early on (#58). It looks like at the time we weren't storing any environment configuration in the repo at all. When did that start?

@chadwhitacre
Copy link
Contributor

The practice of storing settings in a *.env file came in 080e245. Prior to that they were in the Makefile.

@chadwhitacre
Copy link
Contributor

Initial PM between @patcon and I, for the record:

screen shot 2014-04-03 at 6 05 54 pm

@chadwhitacre
Copy link
Contributor

Storing secrets in the Makefile started in 1f82ba3 (part of #99). No defense of the practice from a security standpoint in the commit or on the ticket. It looks like secrets may have been in the README even further back ...

@chadwhitacre
Copy link
Contributor

Environment variables are first mentioned in README in ee215ae, but without values.

@chadwhitacre
Copy link
Contributor

Note in README from ee215ae:

The SAMURAI_* keys are problematic. I need to think about how to handle that
safely. Same with GITHUB_*.

@chadwhitacre
Copy link
Contributor

70113d5 is where I added a GitHub client secret to the README.

@chadwhitacre
Copy link
Contributor

That was the first secret I added to the repo.

@chadwhitacre
Copy link
Contributor

That was in June of 2012. We didn't start logging the #gittip IRC channel until January 2, 2013.

@chadwhitacre
Copy link
Contributor

So there's no chance of reading my rationale at the time for starting to store secrets in the repo.

@chadwhitacre
Copy link
Contributor

Stripe entered the README in 4065d73, but (as with Samurai) without values.

@chadwhitacre
Copy link
Contributor

My memory is that my rationale was that this was a dev app and we asked for no permissions, so even if someone stole creds they couldn't do anything.

@chadwhitacre
Copy link
Contributor

By the time we get to 1f82ba3 (mentioned above as the place where we move environment configuration from the README to the Makefile), we have secrets for Balanced, GitHub, and Twitter. Stripe is still stubbed out (no secret).

@chadwhitacre
Copy link
Contributor

39db3ec is the parent of 1f82ba3.

@chadwhitacre
Copy link
Contributor

The Stripe stub was taken out and a Balanced secret was added in e4b0a0f.

@chadwhitacre
Copy link
Contributor

The Stripe stub was added back in 49d6439 (#226).

@chadwhitacre
Copy link
Contributor

Stripe remains stubbed to this day. We have never leaked a Stripe secret.

@chadwhitacre
Copy link
Contributor

Found Venmo at ff9ab95.

@chadwhitacre
Copy link
Contributor

Found Bountysource at dd011de.

@chadwhitacre
Copy link
Contributor

Found Bitbucket at 09a9570.

@chadwhitacre
Copy link
Contributor

Found Twitter at fc54d00.

P.S. I am checking that the secret introduced at each commit is the one we have in the repo still (eb898d6).

@chadwhitacre
Copy link
Contributor

Found OpenStreetMap at 364b78b.

@chadwhitacre
Copy link
Contributor

IRC, btw.

Also, Twitter.

@chadwhitacre
Copy link
Contributor

I've found one skimpy IRC reference to sharing secrets. Since we were already sharing secrets by the time we moved from README to Makefile (1f82ba3) and that was in November of 2012 and we didn't start logging IRC until January of 2013, I think that we don't have a log of the IRC conversation I remember having about sharing secrets. :-(

@patcon
Copy link
Contributor Author

patcon commented Apr 3, 2014

Not sure if we care going forward, but OpenStreetMaps has a dev api they recommend for testing:
http://wiki.openstreetmap.org/wiki/API_v0.6#URL_.2B_authentication

I can't imagine an openstreetmaps accounts being overly sensitive, but I guess (in respect to future dev users) we should assume it is since we don't know

@erikvanzijst
Copy link
Contributor

I'm trying to understand exactly what happened. I believe the key/secret to some gittip dev account on Bitbucket was leaked through a commit. However, aside from that one bb user account being exposed, what else happened?

@chadwhitacre
Copy link
Contributor

Thanks for dropping by here and in IRC, @erikvanzijst. Circling back, it sounds like this is a false alarm. I would like to understand OAuth better so we can really be sure of this.

@chadwhitacre
Copy link
Contributor

This whole thing gives me the willies, to be honest. Makes me want to stop authenticating via accounts elsewhere and use passwords instead (#1052).

@zbynekwinkler
Copy link
Contributor

This whole thing gives me the willies, to be honest.

I've been trying to understand OAuth for several months (since trying to tackle #715) and I must say I still do not grok all the ramifications. Actually, reading this issue here I am really not sure what the current problem is :(. Much less what a sufficient solution is.

Makes me want to stop authenticating via accounts elsewhere and use passwords instead (#1052).

I can reason about the (non)security of storing passwords. I cannot do the same for OAuth. And I've tried. But I've said as much at the January retreat.

@patcon
Copy link
Contributor Author

patcon commented Apr 7, 2014

OK, so I've found these informative:

If I'm understanding correctly, we're essentially in the same boat as someone releasing a production android or iphone app, as they can't guarantee the secrecy of their consumer keys since the binaries can be decompiled. And since we're only working locally, in each local instance, it's hard to imagine critical tokens leaking, even though we're not talking to our local site via https (which is normally the thing that makes the situation tenable with the previously mentioned mobile apps situation).

I think this would provide the basis for a reasonable question on stackexchange: "What are the security implications of publicizing a consumer secret for sharing between developers doing local website development?"

Having said that, my personal impression is that this is not an issue for us, and we could safely close this issue and reinstate the shared secrets. And in saying that, I'd like to apologize for whatever part I had in our misunderstanding of this situation. I feel really badly about this :(

@seanlinsley
Copy link
Contributor

From my understanding of OAuth, our client ID & secret are useless to an attacker that wants to gain access to people's accounts that have authorized our dev app. That's because the given access token for that user only exists in their local version of the database. The worst someone could do is impersonate our dev apps.

@chadwhitacre
Copy link
Contributor

I'd like to apologize for whatever part I had in our misunderstanding of this situation. I feel really badly about this :(

@patcon Your emotions here are understandable, but please don't feel bad. When you first raised the issue in IRC, I didn't have an immediate good answer for you, so it was perfectly appropriate to raise an alarm. This is not something we should be doing without a thorough understanding of the risks, and whatever conversation we had from when we initially implemented this pattern is lost to the sands of time. Documenting the history of this issue is valuable in its own right, and auditing ourselves on this point is absolutely worth our time. You've done us a service. !m @patcon. :-)

@patcon
Copy link
Contributor Author

patcon commented Apr 7, 2014

:)

@patcon
Copy link
Contributor Author

patcon commented Apr 7, 2014

IRC discussion about using remote postgression db over non-ssl

@patcon patcon mentioned this pull request Apr 7, 2014
@patcon patcon deleted the remove-bitbucket-dev-app branch April 8, 2014 00:11
@patcon
Copy link
Contributor Author

patcon commented Apr 8, 2014

I think this would provide the basis for a reasonable question on stackexchange...

https://security.stackexchange.com/questions/55072/what-are-the-implications-of-publicizing-a-consumer-secret-for-sharing-between-d

@chadwhitacre
Copy link
Contributor

Reopening because I'm not satisfied yet that we've thoroughly documented the risks here.

@chadwhitacre
Copy link
Contributor

Or not. This is a PR so it can't be reopened, at least not ttw.

@chadwhitacre
Copy link
Contributor

There are four protocols in play:

  • OAuth 2.0—GitHub, Venmo
  • OAuth 1.0—Bitbucket, OpenStreetMap, Twitter
  • Bountysource (undocumented; kludged OAuth?)
  • Balanced (the risk is that someone stored a real credit card using this app)

chadwhitacre added a commit that referenced this pull request Apr 14, 2014
We killed the old one during #2239.
@chadwhitacre chadwhitacre mentioned this pull request Apr 14, 2014
chadwhitacre added a commit that referenced this pull request Apr 14, 2014
clone1018 added a commit that referenced this pull request Apr 14, 2014
chadwhitacre added a commit that referenced this pull request Apr 16, 2014
We killed the old one during #2239.
chadwhitacre added a commit that referenced this pull request Apr 16, 2014
@patcon
Copy link
Contributor Author

patcon commented May 12, 2014

I would like to understand OAuth better so we can really be sure of this.

Saw this today and thought of y'all :)
http://www.oauthsecurity.com/

@chadwhitacre
Copy link
Contributor

@patcon Do you see an obvious tl;dr for this ticket?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants