exploitable_typo_in_url_regex__63__.mdwn 4.71 KB
Newer Older
Tails developers's avatar
Tails developers committed
1
The page at [[contribute/design/I2P]] describes a regexp:
127.0.0.1's avatar
127.0.0.1 committed
2

Tails developers's avatar
Tails developers committed
3
urls matching `^http://(127.0.0.1)|(localhost):7657(/.*)?` will get a direct connection to the local host so the I2P router console can be reached.
127.0.0.1's avatar
127.0.0.1 committed
4

Tails developers's avatar
Tails developers committed
5
According to the FoxyProxy regex documentation (http://getfoxyproxy.org/patterns.html), FoxyProxy uses ECMAScript-compatible regexps, so what that regexp actually means is:
127.0.0.1's avatar
127.0.0.1 committed
6
7
8
9
10

Any URL beginning "http://127[any char]0[any char]0[any char]1" OR any URI containing the string "localhost:7657" anywhere within it, will evade the proxy.

It's probably meant to be:

Tails developers's avatar
Tails developers committed
11
	^http://(127\.0\.0\.1|localhost):7657(/.*)?$
127.0.0.1's avatar
127.0.0.1 committed
12
13

Specifically, the changes here are:
Tails developers's avatar
Tails developers committed
14

127.0.0.1's avatar
127.0.0.1 committed
15
16
17
18
1) we escape the wildcard dot characters to accept only a literal dot.

Failing to change this means that a URL like the following would match:

Tails developers's avatar
reply.    
Tails developers committed
19
	http://127x0y0z1/maliciousscript.php:7657
127.0.0.1's avatar
127.0.0.1 committed
20
21
22

So, a machine named 127x0y0z1 on the same LAN could be accessed without the proxy. Not that machine names should begin with numbers, but still...

Tails developers's avatar
Tails developers committed
23
1a) You might also want to consider using `(?:blah1|blah2)` instead of `(blah1|blah2)` for performance reasons, unless you actually need to capture blah1 or blah2 for later use. But that's not a security thing, may make no difference, and may reduce readability/maintainability.
127.0.0.1's avatar
127.0.0.1 committed
24
25
26
27
28

2) We place the "or" character inside the braces, where it separates only the halves of the braced clause, rather than having it separate the entire URL in two.

Failing to change this means that a URL like the following would match:

Tails developers's avatar
reply.    
Tails developers committed
29
	http://example.com/maliciousscript.php?localhost:7657
127.0.0.1's avatar
127.0.0.1 committed
30
31
32

3) Added the final $ anchor, without which the final (/.*)? became meaningless.

127.0.0.1's avatar
127.0.0.1 committed
33
Failing to change this means that URLs like the following would match:
127.0.0.1's avatar
127.0.0.1 committed
34

Tails developers's avatar
reply.    
Tails developers committed
35
	http://localhost:76579
127.0.0.1's avatar
127.0.0.1 committed
36
37
38

or

Tails developers's avatar
reply.    
Tails developers committed
39
	http://localhost:7657?something=bad
127.0.0.1's avatar
127.0.0.1 committed
40
41
42
43

Not a terrible risk, but who can tell what's running on port 76579? So if you want to guarantee anything following the port number is separated by a slash, you need that anchor.


127.0.0.1's avatar
127.0.0.1 committed
44
While we're at it, let's look at the other regexps on that page.
127.0.0.1's avatar
127.0.0.1 committed
45

Tails developers's avatar
Tails developers committed
46
`^https?://[^/]+\.i2p(:[0-9]{1,5})?(/.*)?`
127.0.0.1's avatar
127.0.0.1 committed
47
48

Well, the following would match:
127.0.0.1's avatar
127.0.0.1 committed
49

Tails developers's avatar
reply.    
Tails developers committed
50
	http://malicious.example.com?.i2p
127.0.0.1's avatar
127.0.0.1 committed
51

127.0.0.1's avatar
127.0.0.1 committed
52
That is, a regular .com site could be sent through the .i2p filter. No idea if that could be exploited, but let's fix that up anyway.
127.0.0.1's avatar
127.0.0.1 committed
53

Tails developers's avatar
Tails developers committed
54
	^https?://[-a-zA-Z0-9.]+\.i2p(:[0-9]{1,5})?(/.*)?$
127.0.0.1's avatar
127.0.0.1 committed
55

Tails developers's avatar
Tails developers committed
56
Here, I've made a white-list for the domain name, instead of a blacklist; and again, I've added the terminating `$` anchor so that the `(/.*)?` is meaningful.
127.0.0.1's avatar
127.0.0.1 committed
57

Tails developers's avatar
Tails developers committed
58
Again, the brackets (blah) should probably be non-capturing brackets, like `(?:blah)` for speed, but this reduces readability and maintainability, so I didn't include it above.
127.0.0.1's avatar
127.0.0.1 committed
59
60


127.0.0.1's avatar
127.0.0.1 committed
61
The third regexp looks fine to me.
127.0.0.1's avatar
127.0.0.1 committed
62
63


Tails developers's avatar
Tails developers committed
64
[[todo/FTP_in_Iceweasel]] describes some more regexps. Let's check them, too.
127.0.0.1's avatar
127.0.0.1 committed
65

Tails developers's avatar
Tails developers committed
66
`ftp://.*`
127.0.0.1's avatar
127.0.0.1 committed
67

Tails developers's avatar
Tails developers committed
68
`http(s)?://.*`
127.0.0.1's avatar
127.0.0.1 committed
69

Tails developers's avatar
Tails developers committed
70
These both need an anchor ^ at the beginning, and the ending `.*` seems pointless, since there's no end anchor. Also, the brackets: they do nothing. I'd replace these with:
127.0.0.1's avatar
127.0.0.1 committed
71

Tails developers's avatar
Tails developers committed
72
`^ftp://`
127.0.0.1's avatar
127.0.0.1 committed
73

Tails developers's avatar
Tails developers committed
74
`^https?://`
127.0.0.1's avatar
127.0.0.1 committed
75

Tails developers's avatar
Tails developers committed
76
Finally, there's: `http://[a-zA-Z0-9\.]*\.i2p(/.*)?`
127.0.0.1's avatar
127.0.0.1 committed
77

127.0.0.1's avatar
127.0.0.1 committed
78
That's better than the last .i2p regexp, but you don't need to escape a dot if it's in a group; domain names can include dashes, too; you need to deal with alternative port numbers; and you need beginning and ending anchors. So the version listed earlier is probably a better choice:
127.0.0.1's avatar
127.0.0.1 committed
79

Tails developers's avatar
Tails developers committed
80
	^https?://[-a-zA-Z0-9.]+\.i2p(:[0-9]{1,5})?(/.*)?$
127.0.0.1's avatar
127.0.0.1 committed
81
82
83
84

Of course, that still allows requests for invalid names like "http://...---...i2p:00000" but I assume that's OK and perfectly secure, so long as no valid non-.i2p addresses match.

Many apologies if my English is unclear: please feel free to ask for clarification on any points.
Tails developers's avatar
Tails developers committed
85

Tails developers's avatar
Tails developers committed
86
87
> Thanks! All this was fixed in the devel branch (41ee709).
>> [[done]] in Tails 0.8
Tails developers's avatar
reply.    
Tails developers committed
88
89
90
91
92

>> Thanks for the update, and for fixing up my markup - I learned
>> stuff! :D Is the devel branch publicly available on some SVN
>> somewhere or something?

Tails developers's avatar
Tails developers committed
93
>>> sure, see [[contribute/git]].
Tails developers's avatar
reply.    
Tails developers committed
94
95
96
97
98
99
100
101
102

>> I'd kinda like to doublecheck that the final regexes look good,

>>> great.

>> and
>> to fix the pages here to show the regexes actually used.

>>> I think I've done most of it already.
103
104
105
106
107
108
109
110
111
112
113

>>>> Unfortunately, the result of all these recent changes is a set of
>>>> broken (as in never matching) regexps => FoxyProxy always selects
>>>> the Tor socks proxy. After reading the foxyproxy source, it
>>>> appears the regexps are automatically enclosed between `^` and
>>>> `$` when `isMultiLine="false"`, which is the default and what our
>>>> regexps have been using forever, so I'm going to revert the
>>>> addition of these special chars. Also, I've migrated every regexp
>>>> that could be to a wildcard pattern (commits 9ea7b1 and f73cc7d
>>>> in devel branch) for simplicity's sake; seems to both fix the
>>>> issues you were raising, and... actually work :)