Closed Bug 1392400 Opened 7 years ago Closed 7 years ago

Allow symbol upload by sending a URL to a zip file to fetch instead of uploading zip file contents

Categories

(Socorro :: Symbols, task)

Product:
Socorro
Component:
Symbols
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ted, Assigned: peterbe)

References

Details

Attachments

(1 file)

Link to GitHub pull-request: https://github.com/mozilla-services/tecken/pull/351
51 bytes, text/x-github-pull-request
Details | Review 
In bug 1392349 RyanVM noticed that our Firefox symbol upload tasks were occasionally timing out. They have a 10 minute time limit currently, but the full symbols.zip in the log posted there is 1.1GB, so it takes the task over 4 minutes to download it, then it has to upload the full 1.1GB back to the symbol API.

It seems like it would be faster if the symbol upload task could just give the symbol api the URL to the symbols.zip, and have it fetch it so that we'd at least skip half the network traffic here.
Depends on: 1365672
Greg,

Can you weigh in here. 
Remember, we only give out permissions to trusted people. If one of those people "go rogue" they could really do some damage by uploading nasty symbols that override existing good symbols. But we give out the permissions extremely carefully and manually. 

What could go wrong if instead of clients upload, they do a POST to give us a URL which we proceed to download into memory and upload into our temporary store in S3 before sending to a Celery job to process. 

One low-hanging fix I can image is to make sure the URL is https:// when parsed (or localhost). We could also first do a HEAD request just to make sure its content type is sane but I don't see much value in that because it'll be obvious later if we can or can't extract it as a binary zip. 

Any other ideas/ thoughts?
Flags: needinfo?(gguthe)
The main thing I'd want to avoid is uploaders tricking the app into resolving and exfiltrating data (like AWS instance metadata) via server side request forgery (SSRF). This item from the security checklist applies:

* [ ] Do not proxy requests from users without strong limitations and filtering (see [Pocket UserData vulnerability](https://www.gnu.gl/blog/Posts/multiple-vulnerabilities-in-pocket/)). Don't proxy requests to [link local, loopback, or private networks](https://en.wikipedia.org/wiki/Reserved_IP_addresses#IPv4) or DNS that resolves to addresses in those ranges (i.e. 169.254.0.0/16, 127.0.0.0/8, 10.0.0.0/8, 100.64.0.0/10, 172.16.0.0/12, 192.168.0.0/16, 198.18.0.0/15).

I'd include limiting the url scheme to https:// as part of that and symbol archive validation should make this hard to pull off.

I could also see this possibly making DOS easier via uploading zip bombs to a bunch of servers with more bandwidth to tecken.


Ultimately we still have to trust uploaders like you say, so I think this could work if we:

* add another permission for upload via URL and limit it to a subset of uploaders
* periodically review and revoke that permission (maybe automatically if the API key hasn't been used recently)
* have logging in place for auditing in case someone does overwrite other symbols accidentally or maliciously. This should include: who started an upload (API key, client IP), which url they uploaded, what the url resolved to, hash of what we downloaded, and where we put it.

Nice to haves: 

* whitelisting or blacklisting domains and IPs (e.g. no uploads from infosec's IP blacklist) and file hashes
* some sort of rollback mechanism for a bad upload

We've gotten in trouble running services that proxy random requests from the public, but this should be relatively low risk if it's authenticated and limited to a few trusted people.
Flags: needinfo?(gguthe)
Hmm...

I don't think I get your point about "limiting the url scheme to https:// as part of that and symbol archive validation should make this hard to pull off"
I have a prototype now and once you're auth'ed in with your API token and upload permissions are checked, it checks the URL. And currently it only checks that the URL really is https://.

Not sure what you suggest else. 

With regards to having an extra permission for "upload by download" (i.e. POST'ing a 'url' instead of POST'ing a file content-type form) might not make a whole lot of sense. If you want to create a "bomb" does it matter if you upload it or have Tecken download it to itself?

Some loose ideas to consider (perhaps Ted can chime in too):

* Perhaps should expand the permissions so that certain folks can't OVERWRITE existing symbols unless it was their own upload according to upload records. 
* No zip extraction or URL download happens until after the auth/permission check has been made. 
* Are there other checks we can do on the URL? E.g. download the first X bytes and look for zip header info?
The other simple solution might be to simply do something like this:
Instead of
  
  # settings.py
  enable_upload_by_download = os.environ.get('ENABLE_UPLOAD_BY_DOWNLOAD', False)

  # settings.py
  can_upload_by_download = parselist(os.environ.get('UPLOAD_BY_DOWNLOAD_EMAILS'))

Then we can enable it gently just for our RelEng uploader.
Are the slow uploads mainly coming from a fairly static set of mozilla or other trusted domains? If so, maybe we can just whitelist "upload by download" domains per user or group in settings.py

Alternatively, we could sign the symbol downloads and have trusted public keys in settings.py
(In reply to Greg Guthe [:g-k] from comment #5)
> Are the slow uploads mainly coming from a fairly static set of mozilla or
> other trusted domains? If so, maybe we can just whitelist "upload by
> download" domains per user or group in settings.py
> 

I don't know actually. The context is that there's a piece of code running in TaskCluster which has the API key. 
The source of the symbols.zip file, I don't know. But what this TaskCluster code is something like this:

$ # In actuality, I think it's Python, not bash that does this. 
$ wget https://buildartefacts.example.com/date/symbols.zip
$ curl -X POST -H 'Auth-token: TeckenAPITOKEN' --form file=@symbols.zip https://symbols.mozilla.org/upload/

What they instead want to do is this:

$ # Again, pseudo code
$ curl -X POST -H 'Auth-token: TeckenAPITOKEN' -d url="https://buildartefacts.example.com/date/symbols.zip" https://symbols.mozilla.org/upload/

Internally, tecken will then do:

$ Again, pseudo code
$ wget https://buildartefacts.example.com/date/symbols.zip
$ unzip symbols.zip
$ upload_into_s3 ./symbols

> Alternatively, we could sign the symbol downloads and have trusted public
> keys in settings.py

Instead of my suggestion in comment 4, how about this:

   # settings.py
   can_upload_by_download_domains = ['queue.taskcluster.net']

After all, as you can see in https://bugzilla.mozilla.org/show_bug.cgi?id=1392349#c1 a real example URL would be:    https://queue.taskcluster.net/v1/task/YC0FgOlETmS8P6H4wnKTCg/artifacts/public/build/target.crashreporter-symbols-full.zip
All the uses of this in CI would be posting URLs to taskcluster artifacts at https://queue.taskcluster.net/ as in comment 6. Whitelisting just that domain for this feature would be fine. I (and others) do occasionally upload symbols in other contexts, but we could just continue POSTing the symbols.zip as we are currently.

Signing the zip files sounds like a lot more work, since signing is a completely separate process in our build automation.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> All the uses of this in CI would be posting URLs to taskcluster artifacts at
> https://queue.taskcluster.net/ as in comment 6. Whitelisting just that
> domain for this feature would be fine. I (and others) do occasionally upload
> symbols in other contexts, but we could just continue POSTing the
> symbols.zip as we are currently.
> 

Unless you're TaskCluster/RelEng the simplest might method might just be to sign in to symbols.mozilla.org, click "Upload" and click "Upload Now" and use the web form. That page also has links to instructions for how to do it by curl/Python.
(In reply to Peter Bengtsson [:peterbe] from comment #6)
> 
> Instead of my suggestion in comment 4, how about this:
> 
>    # settings.py
>    can_upload_by_download_domains = ['queue.taskcluster.net']
> 

Sounds good. I mainly want to avoid the ability to download random files from the internet.

queue.taskcluster.net is part of the web bug bounty too so if someone can upload to it that would be another security issue.
Actually, all queue.taskcluster.net URLs redirect (303) to public-artifacts.taskcluster.net.
Commit pushed to master at https://github.com/mozilla-services/tecken

https://github.com/mozilla-services/tecken/commit/9856dbd9d2cc0e33ccd3428a6efefb824a4d7246
Bug 1392400 upload by download (#351)

* fixes bug 1392400 - upload by download

* fixes bug 1392400 - upload by download
By default it whitelists 2 domains:

* queue.taskcluster.net

* public-artifacts.taskcluster.net
Assignee: nobody → peterbe
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
To be able to test this more, can you guide me to get a list of URLs that I can HTTP POST into Tecken stage?
Flags: needinfo?(ted)
I wrote a little Python script to query the Taskcluster index for Firefox builds, and then query the Taskcluster queue for the artifacts from each build and list just the URL to symbols zips:
https://gist.github.com/986e93cb12e49d6b38fd026b463ac979

It only requires `requests`. I hard-coded a limit of 10 zip files in there, but you can obviously set that to whatever you need.
Flags: needinfo?(ted)
Blocks: 1423881
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: