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. 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
