Bug 24788 - Koha::Object->store calls column names as methods, relying on AUTOLOAD, with possibly surprising results
Summary: Koha::Object->store calls column names as methods, relying on AUTOLOAD, with ...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P2 major (vote)
Assignee: David Gustafsson
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 14957
  Show dependency treegraph
 
Reported: 2020-03-03 13:28 UTC by David Gustafsson
Modified: 2020-04-14 22:54 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10


Attachments
Bug 24788: Remove autoloaded column accessors in Koha::Object->store (2.56 KB, patch)
2020-03-03 13:37 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24788: Remove autoloaded column accessors in Koha::Object->store (2.61 KB, patch)
2020-03-03 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 24788: Remove autoloaded column accessors in Koha::Object->store (2.70 KB, patch)
2020-03-06 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24788: Remove autoloaded column accessors in Koha::Object->store (2.68 KB, patch)
2020-03-06 10:31 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 Mapley 2020-04-14 22:54:40 UTC
Depends on enhancement, not backporting to 18.11.x series