(In reply to Fridolin Somers from comment #0) > Field or subfield label "marc_lib" is mostly used raw in templates. > Isn't that a security issue ? It certainly sounds like it based on what you've provided here. It took a bit of time to work out where "lib" comes from in the following line: marc_lib => $tagslib->{$tag}->{$subfield}->{lib}, But it's from C4::Biblio::GetMarcStructure. We'd need to sanitize values there I think, and double-check other places... > And for example, adding a double quote in a mandatory subfield breaks the > record edition page. Do you mean a double quote in the liblibrarian/libopac db fields? Because of the way Koha/UI/Form/Builder/* modules work, we probably can't use a filter like "html", but we could use a scrubber filter. That would work for the HTML, but it does look like there's Javascript in there as well like koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt, which is more painful. I'm struggling to come up with the solution here. (One reason why I want us to have Secure Coding Guidelines so we don't have to make it up from scratch every time.) Because I saw in Koha/UI/Form/Builder/* there were times where marc_lib contained HTML which is maybe why they have raw filters here. I don't know. Lots of checking needs to be done there. (In reply to David Cook from comment #2) > Because of the way Koha/UI/Form/Builder/* modules work, we probably can't > use a filter like "html", but we could use a scrubber filter. I am not sure I understand what you mean. It's using raw since bug 13618 comment 85. I don't really see a good reason to not html escape marc_lib everywhere actually. (In reply to Jonathan Druart from comment #3) > (In reply to David Cook from comment #2) > > Because of the way Koha/UI/Form/Builder/* modules work, we probably can't > > use a filter like "html", but we could use a scrubber filter. > > I am not sure I understand what you mean. > > It's using raw since bug 13618 comment 85. I don't really see a good reason > to not html escape marc_lib everywhere actually. Basically marc_lib isn't "lib" or "lib_opac" straight from the DB in all cases. It's composed of generated HTML. Koha/UI/Form/Builder/Biblio.pm: marc_lib => $tagslib->{$tag}->{$subfield}->{lib}, Koha/UI/Form/Builder/Item.pm: $subfield_data{marc_lib} = "<span title=\"" . $subfield->{lib} . "\">" . $subfield->{lib} . "</span>"; For Biblio.pm I think it's taken straight from the authorized values, but as you can see for Item.pm, the marc_lib is actually a span wrapping that data, so you can't use the "html" filter in the template toolkit without breaking the display output of Koha/UI/Form/Builder/Item.pm (In reply to David Cook from comment #4) > Koha/UI/Form/Builder/Item.pm: $subfield_data{marc_lib} = "<span > title=\"" . $subfield->{lib} . "\">" . $subfield->{lib} . "</span>"; > > For Biblio.pm I think it's taken straight from the authorized values, but as > you can see for Item.pm, the marc_lib is actually a span wrapping that data, > so you can't use the "html" filter in the template toolkit without breaking > the display output of Koha/UI/Form/Builder/Item.pm I'm guessing there's a reason we have the span here instead of somewhere else, but you might have greater insight into this one. (In reply to David Cook from comment #5) > (In reply to David Cook from comment #4) > > Koha/UI/Form/Builder/Item.pm: $subfield_data{marc_lib} = "<span > > title=\"" . $subfield->{lib} . "\">" . $subfield->{lib} . "</span>"; > > > > For Biblio.pm I think it's taken straight from the authorized values, but as > > you can see for Item.pm, the marc_lib is actually a span wrapping that data, > > so you can't use the "html" filter in the template toolkit without breaking > > the display output of Koha/UI/Form/Builder/Item.pm > > I'm guessing there's a reason we have the span here instead of somewhere > else, but you might have greater insight into this one. Hard to tell really... commit 3b6f7a6d06e71662f4d5736383cbf584230694e7 Date: Tue Jan 14 16:45:33 2003 +0000 little improvments (mandatory subfields in item appear red if not filled) - $subfield_data{marc_lib}=$tagslib->{$tag}->{$subfield}->{lib}; - $subfield_data{marc_mandatory}=$tagslib->{$tag}->{$subfield}->{mandatory}; - $subfield_data{marc_repeatable}=$tagslib->{$tag}->{$subfield}->{repeatable}; +# $subfield_data{marc_lib}=$tagslib->{$tag}->{$subfield}->{lib}; + $subfield_data{marc_lib}="<DIV id=\"error$i\">".$tagslib->{$tag}->{$subfield}->{lib}."</div>"; + $subfield_data{mandatory}=$tagslib->{$tag}->{$subfield}->{mandatory}; + $subfield_data{repeatable}=$tagslib->{$tag}->{$subfield}->{repeatable}; There is never a good reason to generate html from the controller anyway. Created attachment 187503 [details] [review] Bug 40818: Remove HTML from marc_lib Created attachment 187504 [details] [review] Bug 40818: Replace raw with html filter for marc_lib The first patch is moving the span to the template. The second one is using the html filter for marc_lib. I think that's all what is needed here. Maybe this is not a security level issue then ? (In reply to Fridolin Somers from comment #10) > Maybe this is not a security level issue then ? I would say it definitely is a security issue. I've been away/busy a bit lately, but I'll try to put together a POC exploit for this to show the problem and prove the solution. It looks like a filter was missed on that first patch Created attachment 187782 [details] [review] Bug 40818: Remove HTML from marc_lib Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 187783 [details] [review] Bug 40818: Replace raw with html filter for marc_lib Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 187784 [details] [review] Bug 40818: Add missing html filter on html_helpers.inc Signed-off-by: David Cook <dcook@prosentient.com.au> (In reply to Jonathan Druart from comment #9) > The first patch is moving the span to the template. The second one is using > the html filter for marc_lib. I think that's all what is needed here. Thanks for that, Jonathan. I've added a patch as a html filter was needed for html_helpers.inc, but with those 3 I think it all looks good now. + <label [% IF subfield.mandatory %]class="required"[% END %]> Wasnt that a pattern that we forbid? Using an IF within the tag. Does it interfere with translation? Created attachment 188053 [details] [review] Bug 40818: Remove HTML from marc_lib Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 188054 [details] [review] Bug 40818: Replace raw with html filter for marc_lib Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 188055 [details] [review] Bug 40818: Add missing html filter on html_helpers.inc Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #19) > + <label [% IF subfield.mandatory %]class="required"[% END %]> > > Wasnt that a pattern that we forbid? Using an IF within the tag. Does it > interfere with translation? It was, yes. No longer valid, see bug 38720 and bug 38714. Can the patches please be rebased for 22.11 Created attachment 188140 [details] [review] Bug 40818: [22.11] Remove HTML from marc_lib Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 188141 [details] [review] Bug 40818: [22.11] Replace raw with html filter for marc_lib For the other branches: Apply "[22.11] Remove HTML from marc_lib". If it does not apply then fix the conflict, it's a one line change. Then run `perl -p -i -e "s/marc_lib \| .raw /marc_lib \| html /g" **/*.tt **/*.inc` to generate the second patch. Thanks so much Jonathan! Pushed to 22.11.x-security for 22.11.32 This has merge conflicts with 24.05.x Please resolve. Created attachment 188531 [details] [review] Bug 40818: [24.05.x] Remove HTML from marc_lib Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 188532 [details] [review] Bug 40818: [24.05.x] Replace raw with html filter for marc_lib Nice work all, pushed to main for 25.11.00 |
Field or subfield label "marc_lib" is mostly used raw in templates. In OPAC templates : > git grep marc_lib -- koha-tmpl/opac-tmpl/ koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-MARCdetail.tt: <td>[% subfiel.marc_lib | $raw %]</td> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-MARCdetail.tt: ><strong>[% subfiel.marc_lib | $raw %]:</strong> [% IF subfiel.isurl %] In staff interface : > git grep marc_lib -- koha-tmpl/intranet-tmpl/ koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc: <label class="required">[% subfield.subfield | html %] - [% subfield.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc: <label>[% subfield.subfield | html %] - [% subfield.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt: <label class="required">[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt: <label>[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt: <label>[% field.marc_lib | html %] ([% field.tag | html %][% field.subfield | html %])</label> koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt: <label>[% field.marc_lib | html %] ([% field.tag | html %][% field.subfield | html %])</label> koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt: label.push("[% subfield_loo.marc_lib | $raw %]"); koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt: title="[% subfield_loo.marc_lib | $raw %]" koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt: [% subfield_loo.marc_lib | $raw %] koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt: label.push("[% subfield_loo.marc_lib | $raw %]"); koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt: label.push("[% subfield_loo.marc_lib | $raw %]"); koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt: title="[% subfield_loo.marc_lib | $raw %]" koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt: [% subfield_loo.marc_lib | $raw %] koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt: <label class="required">[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt: <label>[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt: <label class="required">[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt: <label>[% iteminformatio.subfield | html %] - [% iteminformatio.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/services/itemrecorddisplay.tt: <label class="required">[% iteminfo.subfield | html %] - [% iteminfo.marc_lib | $raw %]</label> koha-tmpl/intranet-tmpl/prog/en/modules/services/itemrecorddisplay.tt: <label>[% iteminfo.subfield | html %] - [% iteminfo.marc_lib | $raw %]</label> Isn't that a security issue ? And for example, adding a double quote in a mandatory subfield breaks the record edition page.