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). NextGen$
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
