* [review-request][PATCH] bitbake: toaster: project use escapejs filter to avoid tag interpretation @ 2014-11-06 16:11 Michael Wood 2014-11-07 16:52 ` Damian, Alexandru 0 siblings, 1 reply; 5+ messages in thread From: Michael Wood @ 2014-11-06 16:11 UTC (permalink / raw) To: toaster When passing the data from the jinja2 template to javascript make sure we escape and parse the JSON to avoid any invalid values being interpreted. Signed-off-by: Michael Wood <michael.g.wood@intel.com> --- bitbake/lib/toaster/toastergui/templates/project.html | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/bitbake/lib/toaster/toastergui/templates/project.html b/bitbake/lib/toaster/toastergui/templates/project.html index 6a81283..00fb2b4 100644 --- a/bitbake/lib/toaster/toastergui/templates/project.html +++ b/bitbake/lib/toaster/toastergui/templates/project.html @@ -335,13 +335,13 @@ angular.element(document).ready(function() { scope.urls.layers = "{% url 'layers' %}"; scope.urls.targets = "{% url 'targets' %}"; scope.urls.importlayer = "{% url 'importlayer'%}" - scope.project = {{prj|safe}}; - scope.builds = {{builds|safe}}; - scope.layers = {{layers|safe}}; - scope.targets = {{targets|safe}}; - scope.frequenttargets = {{freqtargets|safe}}; - scope.machine = {{machine|safe}}; - scope.releases = {{releases|safe}}; + scope.project = JSON.parse ("{{prj|escapejs}}"); + scope.builds = JSON.parse ("{{builds|escapejs}}"); + scope.layers = JSON.parse ("{{layers|escapejs}}"); + scope.targets = JSON.parse ("{{targets|escapejs}}"); + scope.frequenttargets = JSON.parse ("{{freqtargets|escapejs}}"); + scope.machine = JSON.parse ("{{machine|escapejs}}"); + scope.releases = JSON.parse ("{{releases|escapejs}}"); scope.zone1alerts = []; scope.zone2alerts = []; -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [review-request][PATCH] bitbake: toaster: project use escapejs filter to avoid tag interpretation 2014-11-06 16:11 [review-request][PATCH] bitbake: toaster: project use escapejs filter to avoid tag interpretation Michael Wood @ 2014-11-07 16:52 ` Damian, Alexandru 2014-11-11 11:31 ` Michael Wood 0 siblings, 1 reply; 5+ messages in thread From: Damian, Alexandru @ 2014-11-07 16:52 UTC (permalink / raw) To: Michael Wood; +Cc: toaster@yoctoproject.org [-- Attachment #1: Type: text/plain, Size: 2566 bytes --] Hi, Hi, this is a good point you raise here - there are some aspects that need considering, though - the data coming in this page (e.g. prj, builds, etc..) is already coming as JSON, the conversion is done in the view. Here we mark the value as not needing any further escape (through the safe filter) because we know it's already a valid json string. json is already valid javascript code, so we don't need to parse it manually, the browser will interpret it as such. btw, we're not using jinja2 templating engine, we use the built-in django templating engine :) Cheers, Alex On Thu, Nov 6, 2014 at 4:11 PM, Michael Wood <michael.g.wood@intel.com> wrote: > When passing the data from the jinja2 template to javascript make sure > we escape and parse the JSON to avoid any invalid values being interpreted. > > Signed-off-by: Michael Wood <michael.g.wood@intel.com> > --- > bitbake/lib/toaster/toastergui/templates/project.html | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/bitbake/lib/toaster/toastergui/templates/project.html > b/bitbake/lib/toaster/toastergui/templates/project.html > index 6a81283..00fb2b4 100644 > --- a/bitbake/lib/toaster/toastergui/templates/project.html > +++ b/bitbake/lib/toaster/toastergui/templates/project.html > @@ -335,13 +335,13 @@ angular.element(document).ready(function() { > scope.urls.layers = "{% url 'layers' %}"; > scope.urls.targets = "{% url 'targets' %}"; > scope.urls.importlayer = "{% url 'importlayer'%}" > - scope.project = {{prj|safe}}; > - scope.builds = {{builds|safe}}; > - scope.layers = {{layers|safe}}; > - scope.targets = {{targets|safe}}; > - scope.frequenttargets = {{freqtargets|safe}}; > - scope.machine = {{machine|safe}}; > - scope.releases = {{releases|safe}}; > + scope.project = JSON.parse ("{{prj|escapejs}}"); > + scope.builds = JSON.parse ("{{builds|escapejs}}"); > + scope.layers = JSON.parse ("{{layers|escapejs}}"); > + scope.targets = JSON.parse ("{{targets|escapejs}}"); > + scope.frequenttargets = JSON.parse ("{{freqtargets|escapejs}}"); > + scope.machine = JSON.parse ("{{machine|escapejs}}"); > + scope.releases = JSON.parse ("{{releases|escapejs}}"); > > scope.zone1alerts = []; > scope.zone2alerts = []; > -- > 1.9.1 > > -- > _______________________________________________ > toaster mailing list > toaster@yoctoproject.org > https://lists.yoctoproject.org/listinfo/toaster > -- Alex Damian Yocto Project SSG / OTC [-- Attachment #2: Type: text/html, Size: 4032 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [review-request][PATCH] bitbake: toaster: project use escapejs filter to avoid tag interpretation 2014-11-07 16:52 ` Damian, Alexandru @ 2014-11-11 11:31 ` Michael Wood 2014-11-11 17:17 ` Damian, Alexandru 0 siblings, 1 reply; 5+ messages in thread From: Michael Wood @ 2014-11-11 11:31 UTC (permalink / raw) To: Damian, Alexandru; +Cc: toaster@yoctoproject.org Hi, Valid json can contain unescaped markup tags which will break the javascript e.g. if you put your project name to "</script><h2>Hi mum<!-- or worse some javascript --></h2>" the project page will interpret that. http://jsfiddle.net/uLpecL5o/ The escapejs filter will escape all the correct characters the resulting string of the json can then be safely parsed by the browser. If we want to use |safe we really need to be sure that data is safe, which may mean that instead we sanitise it before storing it. Oops yes too long working with jinja2 which is based on django got confused there! Michael On 07/11/14 16:52, Damian, Alexandru wrote: > Hi, > > Hi, this is a good point you raise here - there are some aspects that > need considering, though - > > the data coming in this page (e.g. prj, builds, etc..) is already > coming as JSON, the conversion is done in the view. Here we mark the > value as not needing any further escape (through the safe filter) > because we know it's already a valid json string. > > json is already valid javascript code, so we don't need to parse it > manually, the browser will interpret it as such. > > btw, we're not using jinja2 templating engine, we use the built-in > django templating engine :) > > > Cheers, > Alex > > > > On Thu, Nov 6, 2014 at 4:11 PM, Michael Wood <michael.g.wood@intel.com > <mailto:michael.g.wood@intel.com>> wrote: > > When passing the data from the jinja2 template to javascript make sure > we escape and parse the JSON to avoid any invalid values being > interpreted. > > Signed-off-by: Michael Wood <michael.g.wood@intel.com > <mailto:michael.g.wood@intel.com>> > --- > bitbake/lib/toaster/toastergui/templates/project.html | 14 > +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/bitbake/lib/toaster/toastergui/templates/project.html > b/bitbake/lib/toaster/toastergui/templates/project.html > index 6a81283..00fb2b4 100644 > --- a/bitbake/lib/toaster/toastergui/templates/project.html > +++ b/bitbake/lib/toaster/toastergui/templates/project.html > @@ -335,13 +335,13 @@ angular.element(document).ready(function() { > scope.urls.layers = "{% url 'layers' %}"; > scope.urls.targets = "{% url 'targets' %}"; > scope.urls.importlayer = "{% url 'importlayer'%}" > - scope.project = {{prj|safe}}; > - scope.builds = {{builds|safe}}; > - scope.layers = {{layers|safe}}; > - scope.targets = {{targets|safe}}; > - scope.frequenttargets = {{freqtargets|safe}}; > - scope.machine = {{machine|safe}}; > - scope.releases = {{releases|safe}}; > + scope.project = JSON.parse ("{{prj|escapejs}}"); > + scope.builds = JSON.parse ("{{builds|escapejs}}"); > + scope.layers = JSON.parse ("{{layers|escapejs}}"); > + scope.targets = JSON.parse ("{{targets|escapejs}}"); > + scope.frequenttargets = JSON.parse ("{{freqtargets|escapejs}}"); > + scope.machine = JSON.parse ("{{machine|escapejs}}"); > + scope.releases = JSON.parse ("{{releases|escapejs}}"); > > scope.zone1alerts = []; > scope.zone2alerts = []; > -- > 1.9.1 > > -- > _______________________________________________ > toaster mailing list > toaster@yoctoproject.org <mailto:toaster@yoctoproject.org> > https://lists.yoctoproject.org/listinfo/toaster > > > > > -- > Alex Damian > Yocto Project > SSG / OTC > > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [review-request][PATCH] bitbake: toaster: project use escapejs filter to avoid tag interpretation 2014-11-11 11:31 ` Michael Wood @ 2014-11-11 17:17 ` Damian, Alexandru 2014-11-12 16:54 ` Michael Wood 0 siblings, 1 reply; 5+ messages in thread From: Damian, Alexandru @ 2014-11-11 17:17 UTC (permalink / raw) To: Michael Wood; +Cc: toaster@yoctoproject.org [-- Attachment #1: Type: text/plain, Size: 5587 bytes --] Agreed that this is an XSS security issue - Thank you for demonstrating it ! I wrongly assumed that json.dumps() will HTML-escape the strings passed to it - apparently it lacks the option to do so, even if it's in the JSON spec. I have a modified patch that: - provides Python-to-JSON dump in a template filter - said template filter escapes HTML content, keeping the output JSON-compatible - modifies all occurences of json.dumps in views.py to use this filter In the same patch I have several other security fixes, since the XSS path remained open inside Angular, or for the commands sent to the server. Can you please review the patch at: http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=toaster/master Cheers, Alex On Tue, Nov 11, 2014 at 11:31 AM, Michael Wood <michael.g.wood@intel.com> wrote: > Hi, > > Valid json can contain unescaped markup tags which will break the > javascript e.g. if you put your project name to "</script><h2>Hi mum<!-- or > worse some javascript --></h2>" the project page will interpret that. > > http://jsfiddle.net/uLpecL5o/ > > The escapejs filter will escape all the correct characters the resulting > string of the json can then be safely parsed by the browser. > If we want to use |safe we really need to be sure that data is safe, which > may mean that instead we sanitise it before storing it. > > Oops yes too long working with jinja2 which is based on django got > confused there! > > Michael > > On 07/11/14 16:52, Damian, Alexandru wrote: > >> Hi, >> >> Hi, this is a good point you raise here - there are some aspects that >> need considering, though - >> >> the data coming in this page (e.g. prj, builds, etc..) is already >> coming as JSON, the conversion is done in the view. Here we mark the value >> as not needing any further escape (through the safe filter) because we know >> it's already a valid json string. >> >> json is already valid javascript code, so we don't need to parse it >> manually, the browser will interpret it as such. >> >> btw, we're not using jinja2 templating engine, we use the built-in django >> templating engine :) >> >> >> Cheers, >> Alex >> >> >> >> On Thu, Nov 6, 2014 at 4:11 PM, Michael Wood <michael.g.wood@intel.com >> <mailto:michael.g.wood@intel.com>> wrote: >> >> When passing the data from the jinja2 template to javascript make sure >> we escape and parse the JSON to avoid any invalid values being >> interpreted. >> >> Signed-off-by: Michael Wood <michael.g.wood@intel.com >> <mailto:michael.g.wood@intel.com>> >> >> --- >> bitbake/lib/toaster/toastergui/templates/project.html | 14 >> +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/bitbake/lib/toaster/toastergui/templates/project.html >> b/bitbake/lib/toaster/toastergui/templates/project.html >> index 6a81283..00fb2b4 100644 >> --- a/bitbake/lib/toaster/toastergui/templates/project.html >> +++ b/bitbake/lib/toaster/toastergui/templates/project.html >> @@ -335,13 +335,13 @@ angular.element(document).ready(function() { >> scope.urls.layers = "{% url 'layers' %}"; >> scope.urls.targets = "{% url 'targets' %}"; >> scope.urls.importlayer = "{% url 'importlayer'%}" >> - scope.project = {{prj|safe}}; >> - scope.builds = {{builds|safe}}; >> - scope.layers = {{layers|safe}}; >> - scope.targets = {{targets|safe}}; >> - scope.frequenttargets = {{freqtargets|safe}}; >> - scope.machine = {{machine|safe}}; >> - scope.releases = {{releases|safe}}; >> + scope.project = JSON.parse ("{{prj|escapejs}}"); >> + scope.builds = JSON.parse ("{{builds|escapejs}}"); >> + scope.layers = JSON.parse ("{{layers|escapejs}}"); >> + scope.targets = JSON.parse ("{{targets|escapejs}}"); >> + scope.frequenttargets = JSON.parse ("{{freqtargets|escapejs}}"); >> + scope.machine = JSON.parse ("{{machine|escapejs}}"); >> + scope.releases = JSON.parse ("{{releases|escapejs}}"); >> >> scope.zone1alerts = []; >> scope.zone2alerts = []; >> -- >> 1.9.1 >> >> -- >> _______________________________________________ >> toaster mailing list >> toaster@yoctoproject.org <mailto:toaster@yoctoproject.org> >> https://lists.yoctoproject.org/listinfo/toaster >> >> >> >> >> -- >> Alex Damian >> Yocto Project >> SSG / OTC >> >> --------------------------------------------------------------------- >> Intel Corporation (UK) Limited >> Registered No. 1134945 (England) >> Registered Office: Pipers Way, Swindon SN3 1RJ >> VAT No: 860 2173 47 >> >> This e-mail and any attachments may contain confidential material for >> the sole use of the intended recipient(s). Any review or distribution >> by others is strictly prohibited. If you are not the intended >> recipient, please contact the sender and delete all copies. >> >> > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > -- Alex Damian Yocto Project SSG / OTC [-- Attachment #2: Type: text/html, Size: 8197 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [review-request][PATCH] bitbake: toaster: project use escapejs filter to avoid tag interpretation 2014-11-11 17:17 ` Damian, Alexandru @ 2014-11-12 16:54 ` Michael Wood 0 siblings, 0 replies; 5+ messages in thread From: Michael Wood @ 2014-11-12 16:54 UTC (permalink / raw) To: Damian, Alexandru; +Cc: toaster@yoctoproject.org Yep this works for me. Michael On 11/11/14 17:17, Damian, Alexandru wrote: > Agreed that this is an XSS security issue - Thank you for > demonstrating it ! > > I wrongly assumed that json.dumps() will HTML-escape the strings > passed to it - apparently it lacks the option to do so, even if it's > in the JSON spec. > > I have a modified patch that: > - provides Python-to-JSON dump in a template filter > - said template filter escapes HTML content, keeping the output > JSON-compatible > - modifies all occurences of json.dumps in views.py to use this filter > > In the same patch I have several other security fixes, since the XSS > path remained open inside Angular, or for the commands sent to the server. > > Can you please review the patch at: > > http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=toaster/master > > Cheers, > Alex > > > > > On Tue, Nov 11, 2014 at 11:31 AM, Michael Wood > <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com>> wrote: > > Hi, > > Valid json can contain unescaped markup tags which will break the > javascript e.g. if you put your project name to "</script><h2>Hi > mum<!-- or worse some javascript --></h2>" the project page will > interpret that. > > http://jsfiddle.net/uLpecL5o/ > > The escapejs filter will escape all the correct characters the > resulting string of the json can then be safely parsed by the browser. > If we want to use |safe we really need to be sure that data is > safe, which may mean that instead we sanitise it before storing it. > > Oops yes too long working with jinja2 which is based on django got > confused there! > > Michael > > On 07/11/14 16:52, Damian, Alexandru wrote: > > Hi, > > Hi, this is a good point you raise here - there are some > aspects that need considering, though - > > the data coming in this page (e.g. prj, builds, etc..) is > already coming as JSON, the conversion is done in the view. > Here we mark the value as not needing any further escape > (through the safe filter) because we know it's already a valid > json string. > > json is already valid javascript code, so we don't need to > parse it manually, the browser will interpret it as such. > > btw, we're not using jinja2 templating engine, we use the > built-in django templating engine :) > > > Cheers, > Alex > > > > On Thu, Nov 6, 2014 at 4:11 PM, Michael Wood > <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com> > <mailto:michael.g.wood@intel.com > <mailto:michael.g.wood@intel.com>>> wrote: > > When passing the data from the jinja2 template to > javascript make sure > we escape and parse the JSON to avoid any invalid values being > interpreted. > > Signed-off-by: Michael Wood <michael.g.wood@intel.com > <mailto:michael.g.wood@intel.com> > <mailto:michael.g.wood@intel.com > <mailto:michael.g.wood@intel.com>>> > > --- > bitbake/lib/toaster/toastergui/templates/project.html | 14 > +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git > a/bitbake/lib/toaster/toastergui/templates/project.html > b/bitbake/lib/toaster/toastergui/templates/project.html > index 6a81283..00fb2b4 100644 > --- a/bitbake/lib/toaster/toastergui/templates/project.html > +++ b/bitbake/lib/toaster/toastergui/templates/project.html > @@ -335,13 +335,13 @@ > angular.element(document).ready(function() { > scope.urls.layers = "{% url 'layers' %}"; > scope.urls.targets = "{% url 'targets' %}"; > scope.urls.importlayer = "{% url 'importlayer'%}" > - scope.project = {{prj|safe}}; > - scope.builds = {{builds|safe}}; > - scope.layers = {{layers|safe}}; > - scope.targets = {{targets|safe}}; > - scope.frequenttargets = {{freqtargets|safe}}; > - scope.machine = {{machine|safe}}; > - scope.releases = {{releases|safe}}; > + scope.project = JSON.parse ("{{prj|escapejs}}"); > + scope.builds = JSON.parse ("{{builds|escapejs}}"); > + scope.layers = JSON.parse ("{{layers|escapejs}}"); > + scope.targets = JSON.parse ("{{targets|escapejs}}"); > + scope.frequenttargets = JSON.parse > ("{{freqtargets|escapejs}}"); > + scope.machine = JSON.parse ("{{machine|escapejs}}"); > + scope.releases = JSON.parse ("{{releases|escapejs}}"); > > scope.zone1alerts = []; > scope.zone2alerts = []; > -- > 1.9.1 > > -- > _______________________________________________ > toaster mailing list > toaster@yoctoproject.org <mailto:toaster@yoctoproject.org> > <mailto:toaster@yoctoproject.org > <mailto:toaster@yoctoproject.org>> > https://lists.yoctoproject.org/listinfo/toaster > > > > > -- > Alex Damian > Yocto Project > SSG / OTC > > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > This e-mail and any attachments may contain confidential > material for > the sole use of the intended recipient(s). Any review or > distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > > > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > > > > > -- > Alex Damian > Yocto Project > SSG / OTC > > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-12 16:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-06 16:11 [review-request][PATCH] bitbake: toaster: project use escapejs filter to avoid tag interpretation Michael Wood 2014-11-07 16:52 ` Damian, Alexandru 2014-11-11 11:31 ` Michael Wood 2014-11-11 17:17 ` Damian, Alexandru 2014-11-12 16:54 ` Michael Wood
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.