On Tue, 2014-07-08 at 13:57 +0200, Bert Massop wrote: > 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. >
Unless fproxy/the node restarts... > Now our current implementation is not very secure, especially for a > public gateway. Agreed. But then again, there's very little allowing server side state to be changed that's exposed (content-filter confirm on external link being the obvious). > 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? > Real web-applications tend to derive it from the session identifier... that can be persisted across restarts (the id would be renewed on first access, just like the control against session-fixation) and is short lived. Florent
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
