Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Saturday, July 12, 2014 11:30:00 AM you wrote: > On Sat, 2014-07-12 at 09:11 +0200, xor wrote: > > Thanks for the very short clarification! > > I've used this as a foundation for a pull request which JavaDocs the > > formPassword variable and relevant functions, please review it: > > > > https://github.com/freenet/fred-staging/pull/260 > > Looks good to me. Thanks. I've posted a comment stating that you ACKed this pull request. > > Also, I have a question about the standard form password validation > > function: > > > > https://github.com/xor-freenet/fred-staging/blob/f2ddcb9cd8c44346e74f6f143 > > 13e01f12871d2a2/src/freenet/support/plugins/helpers1/WebInterfaceToadlet.j > > ava#L71 > > > > It first tries to obtain it from a GET-variable, then from POST. > > Isn't it unsafe to pass this around via GET? > > It is; and yes, we should only call req.getPartAsStringFailsafe()... but > doing so will break existing "broken" calls. I've filed a bug for this at [0] I've checked, and WOT doesn't even use this class, it has a copy-pasta of it which is much more powerful. I don't know who uses it. However given that we want performance fixes soon, I won't change WOT to use it any soon. I've filed a bugtracker entry for making WOT use it [1]. I've implemented anti-CSRF in the WOT class now: https://github.com/freenet/plugin-WoT-staging/commit/4aa6cd8416b2a8fd7e21c0a3d018e4835e079351 It passes down a parameter "mayWrite" to WebPage objects which render the actual page. That parameter is only true if the request was POST and the formPassword did validate. Thanks for your help! [0] https://bugs.freenetproject.org/view.php?id=6237 [1] https://bugs.freenetproject.org/view.php?id=6222 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
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Sat, 2014-07-12 at 09:11 +0200, xor wrote: > On Tuesday, July 08, 2014 09:13:14 AM you wrote: > > 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) > > > > NextGen$ > > Thanks for the very short clarification! > I've used this as a foundation for a pull request which JavaDocs the > formPassword variable and relevant functions, please review it: > > https://github.com/freenet/fred-staging/pull/260 > Looks good to me. > Also, I have a question about the standard form password validation function: > > https://github.com/xor-freenet/fred-staging/blob/f2ddcb9cd8c44346e74f6f14313e01f12871d2a2/src/freenet/support/plugins/helpers1/WebInterfaceToadlet.java#L71 > > It first tries to obtain it from a GET-variable, then from POST. > Isn't it unsafe to pass this around via GET? It is; and yes, we should only call req.getPartAsStringFailsafe()... but doing so will break existing "broken" calls. I suggest that we reverse the condition and log a message when we obtained the token from a GET parameter... leave support for a few releases and then drop support for it altogether. 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
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Tuesday, July 08, 2014 09:13:14 AM you wrote: > 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) > > NextGen$ Thanks for the very short clarification! I've used this as a foundation for a pull request which JavaDocs the formPassword variable and relevant functions, please review it: https://github.com/freenet/fred-staging/pull/260 Also, I have a question about the standard form password validation function: https://github.com/xor-freenet/fred-staging/blob/f2ddcb9cd8c44346e74f6f14313e01f12871d2a2/src/freenet/support/plugins/helpers1/WebInterfaceToadlet.java#L71 It first tries to obtain it from a GET-variable, then from POST. Isn't it unsafe to pass this around via GET? 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
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Tue, 2014-07-08 at 13:57 +0200, Bert Massop wrote: > On Tue, Jul 8, 2014 at 11:49 AM, Florent Daigniere > wrote: > > On Tue, 2014-07-08 at 11:13 +0200, Bert Massop wrote: > >> On Tue, Jul 8, 2014 at 11:07 AM, Florent Daigniere > >> wrote: > >> > On Tue, 2014-07-08 at 09:39 +0200, Bert Massop wrote: > >> >> On Tue, Jul 8, 2014 at 9:13 AM, Florent Daigniere > >> >> 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 formPasswor
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Tue, Jul 8, 2014 at 11:49 AM, Florent Daigniere wrote: > On Tue, 2014-07-08 at 11:13 +0200, Bert Massop wrote: >> On Tue, Jul 8, 2014 at 11:07 AM, Florent Daigniere >> wrote: >> > On Tue, 2014-07-08 at 09:39 +0200, Bert Massop wrote: >> >> On Tue, Jul 8, 2014 at 9:13 AM, Florent Daigniere >> >> 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
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Tue, 2014-07-08 at 11:13 +0200, Bert Massop wrote: > On Tue, Jul 8, 2014 at 11:07 AM, Florent Daigniere > wrote: > > On Tue, 2014-07-08 at 09:39 +0200, Bert Massop wrote: > >> On Tue, Jul 8, 2014 at 9:13 AM, Florent Daigniere > >> 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
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Tue, Jul 8, 2014 at 11:07 AM, Florent Daigniere wrote: > On Tue, 2014-07-08 at 09:39 +0200, Bert Massop wrote: >> On Tue, Jul 8, 2014 at 9:13 AM, Florent Daigniere >> 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) >>
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Tue, 2014-07-08 at 09:39 +0200, Bert Massop wrote: > On Tue, Jul 8, 2014 at 9:13 AM, Florent Daigniere > 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 every
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
On Tue, Jul 8, 2014 at 9:13 AM, Florent Daigniere 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
Re: [freenet-dev] The purpose of NodeClientCore.formPassword in comparison to ToadletContext.checkFullAccess()
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) 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
