BillinghamJ on Monzo urges 480k customers to change their pin numbers

Comment on Monzo urges 480k customers to change their pin numbers parent

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

Oh 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

I thought with https, the ISP (or anyone in between) only sees the base url and not params?

ziggity 7y

A 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-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.

hangonhn 7y

AWS 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

That 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.

lol768 7y

This is correct, I've seen the diff of the two Android versions.

K0nserv 7y

Do you have a source for this you could share?

lol768 7y

Monzo 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

ancarda 7y

Can't you send a body with a GET request? Why change the request to POST/something else if GET was a better semantic match?

buro9 7y

It 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

imo 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

You 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.

dijital 7y

Out of interest, where did you hear this from?

Comments