On Tue, Jul 8, 2014 at 11:49 AM, Florent Daigniere <nextg...@freenetproject.org> wrote: > On Tue, 2014-07-08 at 11:13 +0200, Bert Massop wrote: >> 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. >> > > Right. Sorry for misunderstanding your reply. > > There's no security benefit to having an anti-csrf token when data > unpredictable to the attacker is being exchanged. > > The exception is there to cater for two common cases: > - The application doesn't give a session before login: that's freenet's > case (usually it's done for performance reason; as far as we're > concerned in Freenet, we just don't need it) > - The login process is one-way (a single page), that's often cached > client-side (performance again)... and it's very unintuitive to get the > user to login "twice" when the token expires/is invalid. Users do expect > to be able to keep a tab open to a specific application and for it to > "work" when they use it (auto-logout on session expiration will usually > bring them back to the login page). > > I hope the above clarifies why you'd have an exception there. There's > also a case for logout forms to benefit from the same exception (even if > that means opening up DoS avenues).
Thank you for clarifying this. I understand the implications on those two common cases, but I question how relevant they are to Freenet in particular. The drawbacks you describe are true only under the assumption that tokens will expire at some point. AFAIK our current implementation of formPassword makes use of an immutable token that is reused over and over again, hence it will never expire. So under the current implementation, the exception seems unnecessary. Now our current implementation is not very secure, especially for a public gateway. If we were to generate a token based on some initial randomness (e.g. randomness collected on node startup) combined with some identifying information (e.g. IP address or some non-expiring cookie), we'd have a rather secure, client-specific token that invalidates the above drawbacks, right? — Bert _______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
