parse authorization header #486

Closed
dvv opened this Issue Apr 4, 2013 · 16 comments

Projects

None yet

3 participants

@dvv
dvv commented Apr 4, 2013
cowboy_req:parse_header(<<"authorization">>, Req)

uses unguarded base64:mime_decode(D) which can throw when input is bad.

I wonder if it's cowboy's task to protect flow from sudden deaths when input syntactically is ok or such little things left to authors?

Will a patch be welcome?

TIA,
--Vladimir

@dvv
dvv commented Apr 4, 2013

I believe if it's author's problem then

-spec authorization(binary(), binary()) -> {binary(), any()} | {error, badarg}.

should be appended with | throws

@essen
Nine Nines member

There's no specs for throws. Don't misunderstand what no_return() mean, it should only be used if the function never returns.

As for handling this better, sure, why not. But only send a patch if you seen this in production. Cowboy crashes early on plenty of things, and these kind of patches are only welcome if they make the user code cleaner or allow us to remove unwanted logs.

@dvv

I see. The following is right about unwanted logs -- invalid data is treated as no data:

-           authorization_basic_userid(base64:mime_decode(D),
+           authorization_basic_userid(try base64:mime_decode(D) of Any -> Any catch _:_ -> <<>> end,

It costs, however, ticks for legal users. So let it pend for a while IMHO.
BTW, what is that plenty of things?

@essen
Nine Nines member

You didn't answer or I didn't understand. Have you seen this in logs in production?

@dvv

Not seen. It is all about cutting unwanted logs in case of malicious (or simply bad-written) clients.

@essen
Nine Nines member

Then this is a non-issue. Closing, thanks!

@essen essen closed this May 31, 2013
@dvv

You're right, this is just an open attack vector.

@essen
Nine Nines member

No idea what you are talking about.

@dvv

Anyone who wants to make your basic-authorization-aware site logs flooding, may inject bad authorization header to succeed, because every such request will return 500 and leave garbage in logs.

@essen
Nine Nines member

That's why we have lager?

@dvv

It still eats cycles and it binds us to lager.
I still think it's righter to cut the source of problem.

@essen
Nine Nines member

I'm not going to program defensively everywhere and advise everyone to program defensively just to prevent logs from being flooded. This isn't the Erlang way. You really should use lager if you don't already, even if you don't do web stuff. Log flood is a common reason for node death that lager solves.

@dvv

I see the point. Totally agree on not programming defensively the logic, but here we use just not-matching-well library function that just happens to throw instead of returning {ok, Result} / {error, Error}.
I do differ these cases.

But well, decision made, I silence.

@essen
Nine Nines member

The "library function" also throws because it's not programmed defensively, not because it it calls error/1 or throw/1. Look at its source, it's just like how I would write it. If it was actually throwing an error on purpose I'd probably have suggested implementing it ourselves, but this is obviously not necessary.

@danielwhite

Whoops. My mistake for not searching more thoroughly first.

I'm somewhat convinced. Crashing is acceptable, but not necessarily recommended. Per RFC 2617: "If the origin server does not wish to accept the credentials sent with a request, it SHOULD return a 401 (Unauthorized) response."

@essen
Nine Nines member

Sure but you're not rejecting the credentials here, the request is malformed. It results in a 500 which should be a 400 here but we have no way to distinguish the types of crashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment