Bug 24788

Summary: Koha::Object->store calls column names as methods, relying on AUTOLOAD, with possibly surprising results
Product: Koha Reporter: David Gustafsson <glasklas>
Component: Architecture, internals, and plumbingAssignee: David Gustafsson <glasklas>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P2 CC: 1joynelson, hayleypelham, lucas, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10
Bug Depends on:    
Bug Blocks: 14957    
Attachments: Bug 24788: Remove autoloaded column accessors in Koha::Object->store
Bug 24788: Remove autoloaded column accessors in Koha::Object->store
Bug 24788: Remove autoloaded column accessors in Koha::Object->store
Bug 24788: Remove autoloaded column accessors in Koha::Object->store

Description David Gustafsson 2020-03-03 13:28:53 UTC
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.
Comment 1 David Gustafsson 2020-03-03 13:37:43 UTC
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
Comment 2 David Nind 2020-03-03 19:47:58 UTC
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>
Comment 3 Marcel de Rooy 2020-03-06 09:25:13 UTC
(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 :)
Comment 4 Marcel de Rooy 2020-03-06 09:50:59 UTC
QAing
Looking at Object.t now
Comment 5 Marcel de Rooy 2020-03-06 10:12:46 UTC
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>
Comment 6 David Gustafsson 2020-03-06 10:27:56 UTC
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.
Comment 7 David Gustafsson 2020-03-06 10:31:41 UTC
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>
Comment 8 Martin Renvoize 2020-03-06 15:02:46 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Joy Nelson 2020-04-02 16:44:55 UTC
backported to 19.11.x branch for 19.11.05
Comment 10 Lucas Gass 2020-04-09 20:41:35 UTC
backported to 19.05.x for 19.05.10
Comment 11 Hayley Pelham 2020-04-14 22:54:40 UTC
Depends on enhancement, not backporting to 18.11.x series