joshdholtz / Sentry-Android

[Deprecated] Use official "raven-java" library
https://github.com/getsentry/sentry-java
MIT License
180 stars 48 forks source link

Google Developer Warning #57

Closed ensi-labs closed 8 years ago

ensi-labs commented 8 years ago

Hello, I just received an email from google which reads as follows:

Hello Google Play Developer,

We detected that your app(s) listed at the end of this email are potentially leaking credentials used to make network requests (HTTP and FTP).

Please check for cases where you use url-encoded basic access authentication, for example a URL such as https://username:password@www.example.com/. We recommend that you immediately change the credentials and redesign your app to avoid including them.

Next steps

Sign in to your Developer Console and submit the updated version of your app.
Check back after five hours - we’ll show a warning message if the app hasn’t been updated correctly.

Exposed developer credentials can allow an attacker to compromise your systems which puts user data at risk. For other technical questions about the vulnerability, you can post to Stack Overflow and use the tag “android-security.”

We’re here to help

If you feel we have sent this warning in error, you can contact our developer support team.

Regards,

The Google Play Team

This email is for adding sentry in my project. I think that's the type of connection dsn.

Any idea or solution? Thank you very much

joshdholtz commented 8 years ago

@ensi-labs Thanks for letting me know! I will try to get a fix for this out early this week for you.

parindar commented 8 years ago

Hi

I'm having the same issue. Could you please look into this soon and update.

Thanks.

ensi-labs commented 8 years ago

Hi, Is there any possibility of fixing this?

Thanks

cpowell001 commented 8 years ago

I'm also eager to hear an update on this. Thanks.

kosharskyi commented 8 years ago

any updates?

royuen commented 8 years ago

I am also having problem on this. Any possible fix here?

Thanks!

joshdholtz commented 8 years ago

Hello everyone! Sorry about getting to this so late. Working on fixing this right now 👌

joshdholtz commented 8 years ago

I think I fixed it. I uploaded a new APK to Google Play and now just waiting to see if I receive an emails back from them with the similar text that all of you have received. Will keep you updated on if it works and when I release a new version!

joshdholtz commented 8 years ago

EDITED: Changed 1.4.2 to 1.4.3

Alright everyone, version 1.4.3 is should be out there for you all - https://bintray.com/joshdholtz/maven/sentry-android/1.4.3

I did not merge in my fix yet since I'm not sure if this solves the issue for all of you but I did not receive that error/warning message yet from my APK that I have uploaded.

If you could let me know if this works or doesn't work for you that would be great 😊

cpowell001 commented 8 years ago

@joshdholtz I am confused about your fix. It seems like all you've done is introduce a log function that by default will not print out the logs. Are you saying that just not writing out the logs will fix this issue?(Also, if you ever set debug to true, you'll get a stack overflow as the log method is calling itself, I commented on the PR)

Google explained to me that this error was due to leaked credentials because the Sentry DSN contains the basic auth username:password in the url (e.g. https://username:password@app.getsentry.com/project). Did anyone get a similar explanation?

joshdholtz commented 8 years ago

@cpowell001 I was not sure what the actual solution is/was. Things were getting printed out so that seemed like a good place to start. I submitted an app to Google Play with this fix and I have no received anything from Google yet saying things were getting leaked.

The username and password are not actually getting sent up in that URL (as it can be seen here - https://github.com/joshdholtz/Sentry-Android/blob/master/sentry-android/src/main/java/com/joshdholtz/sentry/Sentry.java#L187-L188). The username and password are parsed from the DSN and put into the Sentry header. The URL format that was getting logged out can be seen below.

My guess to the solution was to just hide all logs since that seemed like a decent place for Google thinking things were getting leaked since we aren't actually using the Basic Auth format to send things up to Sentry's API. The Basic Auth format is only used in the DSN so that we can parse it out and format it into the proper Sentry header for authentication.

07-26 08:31:33.983 26611-26611/com.joshdholtz.sentryapp D/Sentry: Request - {"platform":"java","timestamp":"2016-07-26T13:31:33","message":"OMG this works woooo","event_id":"883ad424856a472d9ece88ca6defb1e9","level":"info"}
07-26 08:31:33.983 26611-26790/com.joshdholtz.sentryapp D/Sentry: Sending to URL - https://app.getsentry.com/api/61891/store/
07-26 08:31:33.983 26611-26790/com.joshdholtz.sentryapp D/Sentry: Using http client
07-26 08:31:38.383 26611-26790/com.joshdholtz.sentryapp I/System.out: AsyncTask #1 calls detatch()
07-26 08:31:38.383 26611-26790/com.joshdholtz.sentryapp D/Sentry: SendEvent - 200 {"id":"883ad424856a472d9ece88ca6defb1e9"}
07-26 08:31:38.383 26611-26790/com.joshdholtz.sentryapp D/Sentry: Removing request - 3a11448e-ae67-46bb-9ab2-0d3d148f326f

I pushed a fix this morning cause I realized that 1.4.2 broke stuff but 1.4.3 should work for you. If you can let me know if this fixes things that would be great as I don't really have any other way to verify that it works or doesn't work 😊

cpowell001 commented 8 years ago

@joshdholtz thanks for the feedback. Our next release is scheduled to go out in a week or two, I'll report back then.

joshdholtz commented 8 years ago

@cpowell001 Anytime! ✊

joshdholtz commented 8 years ago

Has anybody been able to verify that 1.4.3 fixes things for them yet? Super interested to see if this fixes things 😇

ensi-labs commented 8 years ago

@joshdholtz today i publish my apk with 1.4.3 version. I'll report anything

joshdholtz commented 8 years ago

@ensi-labs Awesome! You rock ✊

ensi-labs commented 8 years ago

sorry guys but I get the same error alert

joshdholtz commented 8 years ago

@ensi-labs Can you forward the email to me at me@joshholtz.com? I did not receive one with my application that I published yesterday 😔

ensi-labs commented 8 years ago

google has not sent me an email yet. I have an alert google play, like last time googlewarning (sorry for picture language)

cpowell001 commented 8 years ago

When I spoke to Google, they described the error as regarding leaked developer credentials in network requests and pin-pointed the line of code in our app causing the warning as the hard-coded DSN: private static final String DSN = "https://xxxxxxxxxxxxxx:xxxxxxxxxxxx@app.getsentry.com/xxxxx";

@ensi-labs do you also store the DSN in a variable? @joshdholtz I see the same string in your app but not stored in a variable, perhaps this is harder for their inspection tools to pick up on?

If my understanding is correct, Google's concern is that we might be exposing those credentials with that URL. Given the format that Sentry uses for the DSN, I would assume anyone using their service is potentially leaking credentials.

@joshdholtz you say that the username and password are extracted out of that DSN before being sent upstream. So it seems as though the warning may not be too relevant. One idea to appease them might be to set the username, password, and project number on the Sentry client separately, rather than as a URL containing basic auth credentials.

joshdholtz commented 8 years ago

@cpowell001 I actually did not store it as a variable in my app. I hardcoded it into the method. (Which might be why I don't have an alert). I will give that a shot and see what happens today.

I do like the idea of setting them separately if that is the issue. This is pretty weird 😊

joshdholtz commented 8 years ago

I published a new version with the DSN as a private static final variable. I hope I can see this error too. I will still implement another constructor though because yolo 👌

joshdholtz commented 8 years ago

@cpowell001 @ensi-labs I DID get a warning this time once I made a private static final variable that contains the DSN but DID NOT when it was a variable in my function or when hardcoded into the method as an argument... So that is fun 😊

joshdholtz commented 8 years ago

Summary: Not an issue with this library "directly" but an issue in the way that most people probably use this library.

Solution 1): Do not make the DSN a private static final variable because Google sees that as a vulnerability Solution 2): I will make another constructor that takes the parts of a DSN or something cause Google is making this weird 😉

cpowell001 commented 8 years ago

@joshdholtz nice to get to the bottom of this, thanks for all your work!

ensi-labs commented 8 years ago

I have always put the dns hardcoded, like that: Sentry.init(this, "myDSNStringEtcEtc");

I do not understand how does that affect safety. I think google not like the protocol of Sentry. Thanks for all.

marcomorain commented 8 years ago

Google's safety chances are most likely extracting all the acting constants from the APK and finding any that look like basic auth credentials (http://user:password@service).

Since it's an offline check (rather than running the app and logging HTTP accesses) it's very difficult for them to know if such a string used in a HTTP call or not,

I'd say Josh's suggested fix is best - make a contractor to Sentry that takes the component parts of the DSN, and make an explanatory doc explaining what's going on.

ensi-labs commented 8 years ago

I tried the following combinations

Sentry.init(this, "myDSNStringEtcEtc");

private String dsn = "myDSNStringEtcEtc"; Sentry.init(this, dsn);

private static final String DSN = "myDSNStringEtcEtc"; Sentry.init(this, DSN); in all cases I get a warning for that version

cpowell001 commented 8 years ago

Interesting that @joshdholtz got no warnings when using the first example that @ensi-labs tried.

lsuski commented 8 years ago

It is not problem with logging url but with having it in application. Google scans application for occurance of credentials included in url. For public clients public key instead of secret one should be used. Here https://docs.getsentry.com/hosted/clients/javascript/config/#optional-settings are docs for javascript client. I think that for Android we should use the same strategy.

lsuski commented 8 years ago

I haven't had any warnings from Google for 2 months and received one today so I shouldn't rely on that.

lsuski commented 8 years ago

According to this https://github.com/getsentry/sentry/blob/464ea1abe937887ad2441e918d8393fbc8a4337c/src/sentry/web/api.py I don't think it is possible to use public key for Android app so probably we should change the way of passing credentials to Sentry. Instead of url we should pass them directly.