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.
Created attachment 100007 [details] [review] 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
Created attachment 100064 [details] [review] 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>
(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 :)
QAing Looking at Object.t now
Created attachment 100228 [details] [review] 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>
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.
Created attachment 100242 [details] [review] 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>
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x branch for 19.11.05
backported to 19.05.x for 19.05.10
Depends on enhancement, not backporting to 18.11.x series