Security issue with join

[lukejahnke]

require 'addressable/uri'

puts Addressable::URI.join('http://www.example.com/', '/./')
puts Addressable::URI.join('http://www.example.com/', '/')
puts Addressable::URI.join('http://www.example.com/', '/../')
puts Addressable::URI.join('http://www.example.com/', '/.///')
puts Addressable::URI.join('http://www.example.com/', '/.///../')

will output the following

http://www.example.com/
http://www.example.com/
http://www.example.com/
http://www.example.com///
http://www.example.com///../

[kyledrake]

Yeah, this patch needs to go in. @sporkmonger did you look at this?

[sporkmonger]

I did. It hasn't been ignored, I just hadn't gotten around to explaining why I was rejecting it yet.

RFC 3986 has a very specific algorithm for dot segment removal that's given as part of the spec in section 5.2.4. That algorithm is what Addressable uses for dot segment resolution. I'm not inclined to deviate from it or the related algorithms described in section 5.2.2 and section 5.2.3.

http://www.example.com/ and http://www.example.com// are not equivalent and should not be treated as if they are. Even browsers don't strip double-slashes. Addressable does not engage in magic. It implements a spec as precisely as possible with only a few exceptions. I'm not inclined to make this one of them. Additionally, I fail to see how this is a security issue outside of developers who are misusing the tool and failing to take the standard precautions that should be being put into place in any application that deals with relative references.

If this is a problem for you, you are welcome to fork.

[sporkmonger]

Though I'll reopen briefly because normalization on that last example probably should not have returned http://www.example.com///../.

[kyledrake]

I'm not interested and/or sufficiently knowledgeable on this topic to really maintain another fork of what is already a very solid piece of code, however I do think that last example merits some investigation and possibly a fix. I agree and already understand the low security potential of this to people that use better security mechanisms, but I still think this qualifies as a security issue, low-risk as it may be (for well designed apps).

Edit: apologies, didn't see your followup just now.

[sporkmonger]

Yeah, take a look at the associated patch to see what I'm really objecting to.

[lukejahnke]

Thanks for the links. You are correct regarding my patch being incorrect. I wasn't aware of the behaviour of the multiple slashes in the RFC.

As for the security concern, I thought it was a reasonable assumption that someone could (incorrectly) rely on the normalisation to prevent directory traversal attacks.

[sporkmonger]

Well, it should prevent traversal above root:

require 'addressable/uri'

puts Addressable::URI.join('http://www.example.com/', '..')
puts Addressable::URI.join('http://www.example.com/', '/..')
puts Addressable::URI.join('http://www.example.com/', '/../')

Output:

http://www.example.com/
http://www.example.com/
http://www.example.com/

That's the main threat to be concerned about in dot segment resolution.

[lukejahnke]

Yeah, except if the second parameter is user controlled then it could match my last example.

The file system does not treat // as multiple folders so my last example would traverse above the root.

Any idea how far off a patch will be for this issue?

[sporkmonger]

If the second parameter is user-controlled, you can't afford to skip a separate application-level security check to verify you haven't traversed above your web root or whatever sandbox the user should remain within.

[lukejahnke]

Yeah, I totally agree. I was talking about someone incorrectly using Addressable for the application-level security check though.