search

elplatt / seltzer

CRM for hackerspaces
GNU General Public License v3.0
104 stars 50 forks source link

Add permission check to command_set_password(); close #409. #410

Closed elplatt closed 6 years ago

elplatt commented 6 years ago

There was no user_access() check in command_set_password(). This should fix the issue. Can someone review and verify?

chris18890 commented 6 years ago

Oops, my bad! Will do a test build tomorrow & check it. Does command_reset_password need checked as well? Also, not sure if the value of crm_url is correct

elplatt commented 6 years ago

Ah, yes, the user page.

On Mar 1, 2018 5:31 PM, "Chris Murray" notifications@github.com wrote:

@chris18890 commented on this pull request.

In crm/modules/user/user.inc.php https://github.com/elplatt/seltzer/pull/410#discussion_r171715318:

@@ -866,6 +866,12 @@ function command_set_password () { global $db_connect; global $esc_post;

  • // Check permissions
  • if (!user_access('user_edit')) {
  • error_register('Current user does not have permission: user_edit');
  • return crm_url('permissions');

Should it return the permissions page or just the user page?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/410#pullrequestreview-100618862, or mute the thread https://github.com/notifications/unsubscribe-auth/AAS0WEt_DvUqVMBI9lQ7egdqzLhB9sB8ks5taHbOgaJpZM4SZF0G .

elplatt commented 6 years ago

Ah right, it's been a while since I've looked at the code. I believe it should compare the logged in cid to the cid having its password set.

On Fri, Mar 2, 2018 at 7:47 AM, Chris Murray notifications@github.com wrote:

@chris18890 commented on this pull request.

In crm/modules/user/user.inc.php https://github.com/elplatt/seltzer/pull/410#discussion_r171838392:

@@ -866,6 +866,12 @@ function command_set_password () { global $db_connect; global $esc_post;

  • // Check permissions
  • if (!user_access('user_edit')) {
  • error_register('Current user does not have permission: user_edit');
  • return crm_url('permissions');

just spotted that myself, already working on it :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/410#discussion_r171838392, or mute the thread https://github.com/notifications/unsubscribe-auth/AAS0WATYVHbWdkMDxaOf-PjTj2NUzCKzks5taT9lgaJpZM4SZF0G .

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

chris18890 commented 6 years ago

@elplatt @altinukshini think that's it sorted now, would you be happy for me to merge?

chris18890 commented 6 years ago

Will merge at the weekend if there are no objections @elplatt @ramgarden

chris18890 commented 6 years ago

@ramgarden yes, in a nutshell :)

elplatt commented 6 years ago

There are a couple issues with this PR. Some might have been pre-existing.

  1. Using $esc_post["cid"] might delete a user other than the one specified in the argument. So it should stay as $esc_cid.

  2. Both $esc_cid and $esc_post escape for mysql, so we need to do something else for constructing urls. Probably the best thing to do is to use is_numeric() to verify $cid is a number at the top of user_delete and throw an error otherwise.

chris18890 commented 6 years ago

@elplatt

1 - are you talking about line 436? Or one of the others? On 436 it’s not actually doing the delete operation, just updating the info message to use the same variable. 2 - would you want this implemented for all the other similar methods in user/contact/member modules? If so, I’d suggest merging/closing this PR & dealing with it separately/more consistently across the board

elplatt commented 6 years ago

There are two places: 436 and 878.

436 escapes text for mysql and then inserts it into the html output. It would garble the output without fixing the html injection vulnerability. That vulnerability is already covered by issue #408.

878 is more serious. It takes a mysql-escaped POST variable and inserts it into the redirect url, which not only has an injection vulnerability but might be the wrong cid.

The permission check looks good though, so we should probably revert those lines and keep the rest.

On Sun, Mar 18, 2018 at 7:55 PM, Chris Murray notifications@github.com wrote:

@elplatt https://github.com/elplatt

1 - are you talking about line 436? Or one of the others? On 436 it’s not actually doing the delete operation, just updating the info message to use the same variable. 2 - would you want this implemented for all the other similar methods in user/contact/member modules? If so, I’d suggest merging/closing this PR & dealing with it separately/more consistently across the board

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/410#issuecomment-374068306, or mute the thread https://github.com/notifications/unsubscribe-auth/AAS0WIKixTiwU3TRFBKdMjNj-85TjZv0ks5tfvPrgaJpZM4SZF0G .

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

ramgarden commented 6 years ago

Agreed.

On Mon, Mar 19, 2018 at 1:46 PM, Edward L Platt notifications@github.com wrote:

There are two places: 436 and 878.

436 escapes text for mysql and then inserts it into the html output. It would garble the output without fixing the html injection vulnerability. That vulnerability is already covered by issue #408.

878 is more serious. It takes a mysql-escaped POST variable and inserts it into the redirect url, which not only has an injection vulnerability but might be the wrong cid.

The permission check looks good though, so we should probably revert those lines and keep the rest.

On Sun, Mar 18, 2018 at 7:55 PM, Chris Murray notifications@github.com wrote:

@elplatt https://github.com/elplatt

1 - are you talking about line 436? Or one of the others? On 436 it’s not actually doing the delete operation, just updating the info message to use the same variable. 2 - would you want this implemented for all the other similar methods in user/contact/member modules? If so, I’d suggest merging/closing this PR & dealing with it separately/more consistently across the board

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/410#issuecomment-374068306, or mute the thread https://github.com/notifications/unsubscribe- auth/AAS0WIKixTiwU3TRFBKdMjNj-85TjZv0ks5tfvPrgaJpZM4SZF0G .

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/410#issuecomment-374303729, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnBzawB2IsRCYDgz5UHLYTF4EbhB6B2ks5tf-8NgaJpZM4SZF0G .

chris18890 commented 6 years ago

What do you want for line 872 then? $esc_cid or $esc_post[‘cid’]

chris18890 commented 6 years ago

@elplatt @ramgarden happy for me to resolve the conflict & merge in?

ramgarden commented 6 years ago

Make it so.

On Tue, Mar 20, 2018 at 5:19 PM, Chris Murray notifications@github.com wrote:

@elplatt https://github.com/elplatt @ramgarden https://github.com/ramgarden happy for me to resolve the conflict ~& merge in?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/410#issuecomment-374760440, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnBzesjMJNuHfUWAmZ5FTSCw1n-Jrsxks5tgXJTgaJpZM4SZF0G .