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. — Bert _______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
