zaproxy / zap-hud

The ZAP Heads Up Display (HUD)
Apache License 2.0
253 stars 151 forks source link

Ensure no API calls made from target domain #43

Closed psiinon closed 6 years ago

psiinon commented 7 years ago

The HUD is currently broken with 2.7.0 due to the default blocking of access to the API to all but local domains. To fix this I'm planning on changing the HUD add-on to listen on another (random) port on which it will serve all of the HUD content - scripts, CSS, images etc etc. The port will be random to make it harder for sites to detect that ZAP is in use (they'll still be able to detect the HUD pretty easily). As the HUD will be on a local domain it will then be able to call the API with the default settings. A side effect of this is that we'll be able to serve the images that Jquery expects more easily as we wont be restricted to the API structure. I'll aim to work on this next Monday (18th). Can anyone think of any potential problems with this approach, or suggest better alternatives?

g-k commented 7 years ago

Are the API key abilities summarized somewhere?

I was thinking we could inject and require it as a query param for static resources, but it opens the possibility of a site accessing and using the key.

Maybe a CSRF-type token or random subdomain (like sandstorm uses) if changing or intercepting DNS is an option.

I'd worry that a port number might be brute forced (1024-49152) or collide with other services, but it'd probably be fine to start with.

psiinon commented 7 years ago

I'm in the middle of this now. Its nearly working, but I've got a few more things to fix. As well as a random port its also got a random path prefix. As it wll fail silently for invalid urls I think it wont be practical to brute force that. I'll def document it when its all working :)

dscrobonia commented 7 years ago

I'm excited to see what you've come up with! I was thinking the same as @g-k originally. Something like:

  1. ZAP intercepts target domain and injects js
  2. injected code runs in the browser, adding the iframes
  3. the iframes request the needed HUD resources from ZAP (the html, js, css)
  4. ZAP returns the resources to the iframe along with some API Session Token
  5. The API Session Token is used in subsequent calls to the ZAP API

The key detail of all of this being that HUD resources will always be running in the ZAP domain, and thus the target domain could never access the API Session Token. Even if the target domain can add as many iframes to HUD resources as it likes, it could never control or access their data.

I'm sure there is more than one way to go about this, but does that layout make sense? Or am I missing important here?

psiinon commented 7 years ago

The way I'm currently implementing this is:

Problems with this setup:

This is not dissimilar to sandstorms randomly generated hostname. We could do that as well, but I'm not sure what we would gain by it - we would just have to configure this domain to also be able to access the ZAP API. I'm also not sure what we would gain by an API session token - as mentioned above we're using per operation nonces.

Feedback and suggestions appreciated :)

dscrobonia commented 7 years ago

Sounds good! Thoughts:

Target domains will still be able to detect the HUD (I cant think of any way to prevent this)

While we probably can't fully prevent the target site from detecting the HUD we can actually go a decent way in preventing tampering with the injected script. By wrapping the inject.js code with the module pattern we can prevent adversarial javascript from being able to access any of the inject.js variables or functions. So if the random port and path are hardcoded in there, target domain scripts shouldn't be able to view them.

There will probably still be a few less direct ways to tamper with it (using a service worker to intercept the inject.js comes to mind) but we can tackle those threats individually.

The only way in and out of this inject.js script running in the adversarial domain will be via postMessages, where all incoming messages will be checked to confirm they are coming from the zap domain. This should make the inject.js script pretty insulated.

Where inject.js code needs to interact with the target domain is where we will probably open up the attack surface the most. Adding the alert images into the target page, etc...

I'm also not sure what we would gain by an API session token - as mentioned above we're using per operation nonces.

That works too.

HUD pages will be proxied via the standard ZAP proxying, which might slow it down a little? They will definitely appear in ZAP (which can help debugging;)

The reason we would need to proxy the HUD pages so that we can add the nonces, correct? Which means we won't be able to cache the HUD? This might add a pretty big performance hit - curious to see. Also it would making trying to fix the break issue more difficult I think.

Let me know your thoughts, and correct me if any of this incorrect. haha.

psiinon commented 7 years ago

Had another chance to look at this today. One problem is that now the HUD content is on another port to the API it cant currently access it :( Looks like I'll have to change the core to allow sites to be dynamicly added to a CORS header,,,

psiinon commented 6 years ago

OK, new plan. Instead of using a new port I'm thinking of using an additional callback url - these are randomly generated on a per site basis. We'll then translate the url to the suitable API call. This means we wont have to add any CORS headers. One disadvantage of this approach is that if we leak this url to a target page then that could open up the whole api :/ Having said that, we'll still block calls from IP addrs not in the configs, so we'll still have some protection. We could also filter out calls we know wont be made from the HUD (like shutting down ZAP) I'll have a play with this asap, let me know if you have any thoughts...

dscrobonia commented 6 years ago

I like this way much better. It should be pretty easy to keep the api url safe from the target domain, as long as we're in another domain and don't expose it via some unsafe postMessage call or something like that.

White listing the API calls that should be accessible is a good in-depth addition to make it safe.

psiinon commented 6 years ago

Changed the title to reflect the proposed solution :)

psiinon commented 6 years ago

Ah, theres a bug in the core that prevents this from working on remote sites - I'll submit a PR to fix that asap.

psiinon commented 6 years ago

Its this one: https://github.com/zaproxy/zaproxy/pull/4252