Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable10] Dont't expose hashed password in ocs api #34691

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

IljaN
Copy link
Member

@IljaN IljaN commented Mar 5, 2019

Description

Don't expose hashed password in ocs api

Related Issue

Motivation and Context

Mostly hardening. Attacker could brute-force the password if he knows the internal salt and instance-id

How Has This Been Tested?

  • Test manually by creating and accessing password protected link in UI
  • Test manually by changing password in UI
  • Unit-Tests adjusted

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@IljaN IljaN added this to the development milestone Mar 5, 2019
@IljaN IljaN self-assigned this Mar 5, 2019
@IljaN IljaN force-pushed the stable10_dont_expose_hashed_password_oc branch from 339ba7c to a1206cd Compare March 8, 2019 10:56
@codecov
Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #34691 into stable10 will decrease coverage by 18.97%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             stable10   #34691       +/-   ##
===============================================
- Coverage          64%   45.03%   -18.98%     
===============================================
  Files            1276      116     -1160     
  Lines           75801    11465    -64336     
  Branches         1291     1291               
===============================================
- Hits            48515     5163    -43352     
+ Misses          26907     5923    -20984     
  Partials          379      379
Flag Coverage Δ Complexity Δ
#javascript 53.22% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 30.64% <ø> (-34.51%) 0 <ø> (-19247)
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1151 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff75188...a1206cd. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #34691 into stable10 will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             stable10   #34691   +/-   ##
===========================================
  Coverage          64%      64%           
  Complexity      19247    19247           
===========================================
  Files            1276     1276           
  Lines           75801    75801           
  Branches         1291     1291           
===========================================
  Hits            48515    48515           
  Misses          26907    26907           
  Partials          379      379
Flag Coverage Δ Complexity Δ
#javascript 53.22% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.15% <100%> (ø) 19247 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
apps/files_sharing/lib/API/Share20OCS.php 93.48% <100%> (ø) 184 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff75188...31b47dc. Read the comment docs.

@IljaN IljaN force-pushed the stable10_dont_expose_hashed_password_oc branch from a1206cd to 31b47dc Compare March 8, 2019 13:52
@IljaN IljaN changed the title [WIP][stable10] Dont't expose hashed password in ocs api [stable10] Dont't expose hashed password in ocs api Mar 8, 2019
@IljaN IljaN requested review from VicDeo and jvillafanez March 8, 2019 15:28
@PVince81
Copy link
Contributor

PVince81 commented Mar 8, 2019

@IljaN what did you test exactly ? just the API calls or did you retest the share link dialog to see if it still detects a password presence / absence ? if not, please also test the latter

@IljaN
Copy link
Member Author

IljaN commented Mar 8, 2019

@PVince81 Works as expected (see top-post)

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PVince81 PVince81 merged commit 691a8b8 into stable10 Mar 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the stable10_dont_expose_hashed_password_oc branch March 8, 2019 17:15
@PVince81
Copy link
Contributor

PVince81 commented Mar 8, 2019

@IljaN please port to master

@phil-davis
Copy link
Contributor

Forward port to master is PR #34726

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants