5 ms·
It wasn't the app logging the PIN, was their AWS ELB setup. Two APIs were accepting the PIN as URL parameters on GET requests, since in terms of REST principle
by BillinghamJ 7y ago
It wasn't the app logging the PIN, was their AWS ELB setup.
Two APIs were accepting the PIN as URL parameters on GET requests, since in terms of REST principles, the operations were to retrieve information.
They were changed to non-GETs with the PIN sent in the body instead. The apps needed to be updated to switch to the new APIs.
- buro9 7y agoOh wow. That's worse than accidental logging. Engineers should know the GET params get logged fairly routinely and shouldn't be used for anything sensitive.
- mcny 7y agoI thought with https, the ISP (or anyone in between) only sees the base url and not params?
- hangonhn 7y agoAWS ELBs typically terminate the SSL connection for you. If you’re using AWS’ certificates management, you won’t have access to the private keys so you have to terminate at the ELBs.
- mey 7y agoThat is correct (assuming https isn't compromised). The issue is that once the requests hits your systems, nothing by default treats url paths as sensitive. The end up in you your elb/Apache/nginx/stack trace logs. Making them really accessible to most employees.
- ziggity 7y agoA person in the middle can see the hostname via SNI. But if you're terminating TLS on behalf of a customer, you can see everything. e.g. https://new.blog.cloudflare.com/terminating-service-for-8chan/ https://new.blog.cloudflare.com/terminating-service-for-8cha... > Among other things, that resulted in us cooperating around monitoring potential hate sites on our network and notifying law enforcement when there was content that contained an indication of potential violence. That indicates deep inspection of traffic going through CloudFlare.
- ancarda 7y agoCan't you send a body with a GET request? Why change the request to POST/something else if GET was a better semantic match?
- lol768 7y agoYou can, but it's unusual. Probably best to implement the most obvious fix, particularly because a lot of libraries (and engineers) will associate data with the query string in the context of a GET request. Principle of least surprise and all that.
- buro9 7y agoIt should have been a POST. Idempotency and implications for which operations can be cached is why. GETs are cacheable, but if a PIN can change so can the answer. This is an authentication action, and it should have been POST.
- BillinghamJ 7y agoimo this is exactly the sort of situation where if you can't implement perfectly "clean" REST, you shouldn't try at all and just use a simplified RPC mechanism only instead. For my company, it's all POST-only, no URL params allowed, all inputs as JSON in the body only, no resource IDs in the path - just the method & version only.
- lol768 7y agoThis is correct, I've seen the diff of the two Android versions.
- K0nserv 7y agoDo you have a source for this you could share?
- lol768 7y agoMonzo will post a technical write-up soon. But, in the meantime: Before: .method public abstract pan(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Lio/reactivex/Single; .param p1 # Ljava/lang/String; .annotation runtime Lretrofit2/http/Query; value = "account_id" .end annotation .end param .param p2 # Ljava/lang/String; .annotation runtime Lretrofit2/http/Query; value = "card_id" .end annotation .end param .param p3 # Ljava/lang/String; .annotation runtime Lretrofit2/http/Query; value = "challenge_type" .end annotation .end param .param p4 # Ljava/lang/String; .annotation runtime Lretrofit2/http/Query; value = "challenge" .end annotation .end param .param p5 # Ljava/lang/String; .annotation runtime Lretrofit2/http/Query; value = "idempotency_key" .end annotation .end param .annotation system Ldalvik/annotation/Signature; value = { "(", "Ljava/lang/String;", "Ljava/lang/String;", "Ljava/lang/String;", "Ljava/lang/String;", "Ljava/lang/String;", ")", "Lio/reactivex/Single<", "Lcom/monzo/card/data/api/PanResponse;", ">;" } .end annotation .annotation runtime Lretrofit2/http/GET; value = "card/pan" .end annotation .end method After (v2.59.1 has the fix): .method public abstract pan(Lcom/monzo/card/data/api/RetrievePanRequest;)Lio/reactivex/Single; .param p1 # Lcom/monzo/card/data/api/RetrievePanRequest; .annotation runtime Lretrofit2/http/Body; .end annotation .end param .annotation system Ldalvik/annotation/Signature; value = { "(", "Lcom/monzo/card/data/api/RetrievePanRequest;", ")", "Lio/reactivex/Single<", "Lcom/monzo/card/data/api/PanResponse;", ">;" } .end annotation .annotation runtime Lretrofit2/http/PUT; value = "card/retrieve-pan" .end annotation .end method
- dijital 7y agoOut of interest, where did you hear this from?