[Bug 24788] New: Koha::Object->store calls column names as methods, relying on AUTOLOAD, with possibly surprising results
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Bug ID: 24788 Summary: Koha::Object->store calls column names as methods, relying on AUTOLOAD, with possibly surprising results Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: glasklas@gmail.com QA Contact: testopia@bugs.koha-community.org Koha::Object->store accesse sDBIx::Class::Row columns as $self->$col, using AUTOLOAD, which in effect makes column names and methods share the same namespace. I'm no security expert, but I think this should be considered bad practice and could be an attack vector. It also has huge potential for causing strange bugs, which happened in Bug 14957 where a column is named "delete", calling the "delete" method instead of fetching the column value as intended. Here is patch to fix this using a separate namespace for setting/getting columns through get_column/set_column instead. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14957 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14957 [Bug 14957] Write protecting MARC fields based on source of import -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 100007 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100007&action=edit Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This has security implications and could also be a source of strange bugs. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |glasklas@gmail.com |ity.org | Priority|P5 - low |P2 Severity|enhancement |major Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100007|0 |1 is obsolete| | --- Comment #2 from David Nind <david@davidnind.com> --- Created attachment 100064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100064&action=edit Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This has security implications and could also be a source of strange bugs. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Gustafsson from comment #0)
Koha::Object->store accesse sDBIx::Class::Row columns as $self->$col, using AUTOLOAD, which in effect makes column names and methods share the same namespace. I'm no security expert, but I think this should be considered bad practice and could be an attack vector. It also has huge potential for causing strange bugs, which happened in Bug 14957 where a column is named "delete", calling the "delete" method instead of fetching the column value as intended. Here is patch to fix this using a separate namespace for setting/getting columns through get_column/set_column instead.
In this case it comes from: my $columns_info = $self->_result->result_source->columns_info; just a few lines earlier. So I guess the security argument does not really apply here. And the "huge potential for bugs" is a bit overkill. But the change looks good to me :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QAing Looking at Object.t now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100064|0 |1 is obsolete| | --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 100228 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100228&action=edit Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This has security implications and could also be a source of strange bugs. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 --- Comment #6 from David Gustafsson <glasklas@gmail.com> --- Yes you are correct, I was incorrect about being security issues since only existing columns are looped over. I for some reason got it in my head that it was the columns passed to the store method. I will remove this remark from the commit message. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100228|0 |1 is obsolete| | --- Comment #7 from David Gustafsson <glasklas@gmail.com> --- Created attachment 100242 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100242&action=edit Bug 24788: Remove autoloaded column accessors in Koha::Object->store Columns are accessed as methods, relying on AUTOLOAD, in Koha::Object->store. This could be a source of strange bugs if a column name clashes with an existing method name. To test: 1) Apply patches for Bug 14957 2) Follow the testing instructions, when saving a new marc rule and error is thrown. 3) Apply patch 4) Try saving a new rule once again, this should now work Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com Status|Pushed to master |Pushed to stable Version(s)|20.05.00 |20.05.00, 19.11.05 released in| | --- Comment #9 from Joy Nelson <joy@bywatersolutions.com> --- backported to 19.11.x branch for 19.11.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to stable |Pushed to oldstable Version(s)|20.05.00, 19.11.05 |20.05.00, 19.11.05, released in| |19.05.10 --- Comment #10 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 19.05.x for 19.05.10 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24788 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |hayleymapley@catalyst.net.n | |z --- Comment #11 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Depends on enhancement, not backporting to 18.11.x series -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org