On Tue, Jul 8, 2014 at 11:07 AM, Florent Daigniere <nextg...@freenetproject.org> wrote: > On Tue, 2014-07-08 at 09:39 +0200, Bert Massop wrote: >> On Tue, Jul 8, 2014 at 9:13 AM, Florent Daigniere >> <nextg...@freenetproject.org> wrote: >> > >> > On Tue, 2014-07-08 at 06:23 +0200, xor wrote: >> > > While porting Freetalk code to WOT, I was wondering about why page >> > > rendering >> > > code which does "writes" checks whether the request type is "POST" >> > > - by "writes" I mean stuff which changes anything about the Freetalk >> > > database >> > > such as posting a message. >> > > >> > > The "blame" feature of Git shows that it came from this commit: >> > > >> > > https://github.com/freenet/plugin-Freetalk-staging/commit/ea251b3957cb217fc59284f5d9ab5500dd66f728 >> > > >> > > I suppose the goal of this commit was to ensure that the higher-level >> > > code had >> > > checked whether the request contained a valid formPassword: It only does >> > > the >> > > password-check for POST, not for GET. See: >> > > https://github.com/freenet/plugin-Freetalk-staging/blob/ea251b3957cb217fc59284f5d9ab5500dd66f728/src/plugins/Freetalk/ui/web/WebInterfaceToadlet.java >> > > >> > > This made me wonder about WHY the node has formPassword at all. To >> > > understand >> > > that lets examine which ways can be used to restrict access to a node: >> > > The access controls which the node offers are IP-based only. We have two >> > > configuration options: >> > > - Which IPs can access the node >> > > - Which IPs are allowed "full access". Internally, this can be validated >> > > when >> > > processing a request via ToadletContext.checkFullAccess(). >> > > >> > > Those two options seem to target the same goal as the formPassword >> > > mechanism: >> > > Web interface code usually only allows the user to "modify" stuff if he >> > > has >> > > full access. And the formPassword code also does that as we have seen in >> > > the >> > > above Freetalk code. >> > > >> > > This made me wonder why we HAVE the formPassword if checkFullAccess can >> > > do the >> > > same thing. So I grepped the source code and it turns out that there is >> > > only >> > > one write access to the NodeClientCore.formPassword variable: In the >> > > constructor of NodeClientCore. >> > > If I am correct with the assumption that NodeClientCore is only created >> > > once >> > > at startup and continues to live during the whole run of the node, then >> > > formPassword cannot do anything which checkFullAccess() cannot do >> > > because it >> > > never changes. In fact it isn't any access control at all because if you >> > > obtain formPassword ONCE at the beginning of the lifetime of the node, >> > > it will >> > > always be valid, even if the IP-address access options are changed to >> > > your >> > > disadvantage. >> > > >> > > So the only conclusion is that formPassword is unfinished code. Is that >> > > right? >> > > And code which does not validate it is NOT dangerous yet as long as it >> > > validates checkFullAccess(). Right as well? >> > > >> > > I suppose it was meant to be used as a foundation for a true >> > > Username/Password >> > > login to the node, which was never implemented. Then it would be needed >> > > in >> > > addition to IP-based checkFullAccess() because we would use the IPs to >> > > restrict who can register a username and then do further restrictions >> > > based on >> > > the user's account. >> > > Also it seems to be some sort of emulation of session cookies, and >> > > probably >> > > was implemented this way because someone was paranoid that users would >> > > disable >> > > cookies in their browser. >> > > Am I right with this interpretation of the purpose of formPassword? >> > > >> > > If you can clear me up on what formPassword aims to do, I then might be >> > > able >> > > to: >> > > - Improve its JavaDoc >> > > - Investigate whether it can be replaced with the session cookie code >> > > which I >> > > had implemented for Freetalk/WOT. This code was implemented *after* >> > > formPassword was already there, so it sort of duplicates it. >> > > - Maybe remove the ugly "only modify stuff if the request is POST" check >> > > in >> > > Freeetalk/WOT because its very non-self-explanatory. However we probably >> > > would >> > > have to mark formPassword as deprecated to ensure that people don't >> > > suddenly >> > > change fred to actually use it for access control - then the client >> > > application code would be insecure if it doesn't check for POST. >> > > >> > > Thanks for your help :) >> > >> > The name of the variable is badly chosen: formPassword is an anti-CSRF >> > token (https://www.owasp.org/index.php/Cross-Site_Request_Forgery_% >> > 28CSRF%29 ); do *NOT* touch it. >> > >> > As for when to use one, two rules: >> > 1) if you're changing server side state, you need a POST request >> > 2) all POST requests need an anti-CSRF token (the exception being a >> > login page, where credentials -that are unpredictable to an attacker- >> > are exchanged) >> >> Different code paths for the same thing introduces complexity and >> allows for mistakes, don't go that way. There's no need for this >> exception: just use an anti-CSRF token everywhere and check it as >> early as possible. >> > > You can't. > > GET requests have different semantics; if you use the same code (which > in our case is per node-instance) you will leak it (referrer, ...); and > that will defeat the purpose. Stick to the rules layed above.
I was referring to the exception you explicitly mentioned (i.e. the login page), not the rules in general. To clarify: it's rather obvious that anti-CSRF tokens can't be used on GET requests, but IMHO all POST requests should use them (for the aforementioned reasons). There's no reason not to, even if credentials are deemed unpredictable. — Bert _______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
