Skip to content

[Security] Storing password as plain text #444

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.

Sign up for GitHub

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

Jump to bottom
Closed
PawlikMichal25 opened this issue Sep 15, 2020 · 11 comments
Closed

[Security] Storing password as plain text #444

PawlikMichal25 opened this issue Sep 15, 2020 · 11 comments

Comments

@PawlikMichal25
Copy link
Contributor

I just realised that the library is saving ParseUser data on most of successful user operations, which might seem handy, but the problem is that it is stored as plain text, including password, which is definitely a security issue.

I believe we should change library's behaviour here or at least make it more clear that the library is storing user passwords as plain text.
What do you think?

Or am I missing something? I know that encryption can be turned on in CoreStore library, but right now that would be suboptimal solution, because all data would be automatically encrypted.
@RodrigoSMarques
Copy link
Contributor

Hi @BaranMichal25

Using Sembast, encryption is enabled by default, and you can enter an encryption password.

     ...
      coreStore: await CoreStoreSembastImp.getInstance(password: 'password'),
     ...

@PawlikMichal25
Copy link
Contributor Author

PawlikMichal25 commented Sep 15, 2020 •
edited
Loading

Yes, but then all data will be encrypted, right?
In which case, also loading all other classes and their data will be needlessly slow

@RodrigoSMarques
Copy link
Contributor

Yes, all data will be encrypted.

Is it really slow? How much data will you store locally?
Sembast is a pure dart database and very fast in performance tests.

@fischerscode
Copy link
Contributor

CoreStoreSembastImp is always encrypted (except on web), the password has a default value.

I think, the password should not be stored in any database neither in clear text, nor encrypted. Only storing a hashed password is acceptable.

When inspecting my saved user object (ParseUser.currentUser()), it indeed still contains the password.

I have three solutions to solve this problem:

  1. provide the password when calling login()
  2. storing the password in a field (instead of _objectData) (should be the easiest to implement)
  3. removing the password at the saveInStorage call

I think the second one is the best. I will create a branch to test this change, soon.

fischerscode added a commit to fischerscode/Parse-SDK-Flutter that referenced this issue Sep 16, 2020
@fischerscode
store password in field
ac55d5a 
Potential fix for parse-community#444
@fischerscode
Copy link
Contributor

fischerscode commented Sep 16, 2020 •
edited
Loading

I have implemented option two in this branch.
Testing is highly appreciated as I do not know which side effects this change has.
In a short test, everything seems to work as expected.

@fischerscode fischerscode mentioned this issue Sep 16, 2020
Merged
fischerscode added a commit to fischerscode/Parse-SDK-Flutter that referenced this issue Sep 17, 2020
@fischerscode
store password in field
5c231fa 
Potential fix for parse-community#444
@fischerscode
Copy link
Contributor

I have implemented option two in this branch.

I tested this change in my project and I didn't encounter any problems.

fischerscode added a commit that referenced this issue Sep 17, 2020
@fischerscode
ParseUser: store password in field (#446)
b9f51a6 
* store password in field

Potential fix for #444

* remove password from clone
fischerscode added a commit to fischerscode/Parse-SDK-Flutter that referenced this issue Sep 17, 2020
@fischerscode
ParseUser: store password in field (parse-community#446)
ffa7555 
* store password in field

Potential fix for parse-community#444

* remove password from clone
@fischerscode
Copy link
Contributor

Option 2 was merged with #446 into release/1.0.28.

@PawlikMichal25
Copy link
Contributor Author

Nice, thanks for taking care of it :)

@PawlikMichal25
Copy link
Contributor Author

Actually, can you please explain how this works?
I would think that password is part of objectData, so we're still storing it in the database? Or is it hashed at this point?

@fischerscode
Copy link
Contributor

The password is now stored here and this variable never gets stored in the CoreStore.
(Note: #446 was merged into release/1.0.28. Master does currently not have this change.)

@PawlikMichal25
Copy link
Contributor Author

All righty, thanks again :)

@PawlikMichal25 PawlikMichal25 mentioned this issue Sep 23, 2020
Closed
fischerscode added a commit that referenced this issue Oct 11, 2020
@fischerscode @phillwiggins
@nstrelow @kwizner
Release/1.0.28 (#460)
e35d28d 
Change log:
## 1.0.28
- Added full web support
- split this package in a dart and a flutter one
  - [flutter package](https://pub.dev/packages/parse_server_sdk_flutter)
  - [dart package](https://pub.dev/packages/parse_server_sdk)
- Moved ParseHTTPClient to Dio [#459](#459)
- Bug fixes
- general improvements

Commits:
* Release 1.0.28 - Create new release branch
   - Update references to 1.0.28
* Replace devicelocale by Platform.localeName call (#430)
* Replace devicelocale by Platform.localeName call
* Use flutter_test to meet new requirements
* Livequery: combine both implementations (#433)
* make livequeries web implementation more similar to the mobile implementation
* combining both livequery implementations
* make livequery "part of flutter_parse_sdk"
* smal fix
* Seperate the dart and the flutter parts of this sdk (#434)
* Set TODO's
   Set TODO's, needed for #78 (comment)
* First step of seperating the flutter and the dart parts
* added one todo
* remove connectivity from the dart part
* Removed path_provider from the dart part
   Not so happy with the fact that `CoreStoreSembastImp implements sdk.CoreStoreSembastImp` in `parse_server_sdk.dart` but I didn't find a better solution.
* Removed shared_preferences from the dart part
* Update pubspec.yaml
* Update parse_server_sdk_dart.dart
* removed dart:ui from dart part
* removed flutter from dart part
* fixed tests
* Add WEB-support for CoreStoreSembastImp (#436)
* Add websupport for CoreStoreSembastImp
* Add debug warning when using sembast on web
  ParseCoreData().debug should be null at that time
* fixed issues
* fix getInstance of CoreStoreSembastImp (#438)
* Reset _isConnected var
   If state is equal to ConnectivityResult.none, then there is no connection with LiveQuery server (for example when internet is disconnected). _setReconnect function will now retry connection.
* Fixed bad type
   ConnectivityResult to ParseConnectivityResult
* Preparing the repository for seperated dart and flutter package (#439)
* split code into package directories
* removed unused dependencies
* Updated READMEs
* Small changes and improvements (#441)
* docs changes
* remove flutter_test
* CoreStore was implemented twice
* implemented CoreStoreMemoryImp (default corestore for dart)
* fix tests
* Update .travis.yml
* Fix links (#443)
* fix flutter logo
* fixed flutter link
* Fix infinite connecting on web
* delete old example folder
* fixing travis tests (#447)
  Should work again.
* Fixed throw in live list stream (#448)
* ParseUser: store password in field (#446)
* store password in field
   Potential fix for #444
* remove password from clone
* fix for issue #456 (#457)
   Added @BaranMichal25 fix for #456.
   #456 (comment)
* LiveList: preloadedColumns (#458)
* LiveList: added preloadedColumns
* LiveList: fixed bug with lazyloading=false
* formatting
* Add README section for flutter
* creat migration guide for version 1.0.28 (#445)
* Create migrate-1-0-28.md
* Update migrate-1-0-28.md
* Update migrate-1-0-28.md
* Update migrate-1-0-28.md
* Merge from upstream/release/1.0.28
* fix typos
* added "changed network library" to migration guide
* Move ParseHTTPClient to Dio (#459)
* Merged manually from branch dio
* fixing nullpointer
* Fix batch operations
   Maybe dio changes the toString behaviour.
* use mine
* keep content-type
* fix file upload issue
* fixed file uppload on web
* performance improvement in parse_file_web
* fix for issue #456 (#457)
   Added @BaranMichal25 fix for #456.
   #456 (comment)
* fix flutter test
* fix crash when debug=true
* fix dart test
* add ProgressCallback in README
* added progressCallback example
* improve readme (#468)
* Update CHANGELOG.md (#462)
* allow signup without email (#469)
* Update migrate-1-0-28.md (#471)
   Added section for tests at migration guide.
   Fix: #385 (comment)
* SignUp encoding data to JSON (#470)
* signup - use toJson
  Co-Authored-By: itmesh <[email protected]>
* fix password update
* Add disclaimer to not use Masterkey on client side (#474)
* Add disclaimer to not use Masterkey on client side
* LiveList: fix setState after dispose (#475)
* Querybuilder: whereMatchesKeyInQuery bugfix (#476)
   Fix bug where whereMatchesKeyInQuery cant be used when using different object classes.
* fixed liveListRetryIntervals in flutter
   Not sure how this line got lost.
* Rel 1.0.28: Add license to sub-projects
* Rel 1.0.28: Add dependencies required for rel
* 1.0.28/fix publish warnings (#472)
* added licenses
   I've just copied the LICENSE file.
* change dependencies at dart/pubspec.yaml
* change dependencies at flutter/pubspec.yaml
* flutter: added dio to pubspec.yaml
   It's contained in the pub.spec archive, so I add it here.

Thanks to:
Co-authored-by: Phill <[email protected]>
Co-authored-by: Nils Strelow <[email protected]>
Co-authored-by: FNPCMDs <[email protected]>
Co-Authored-By: itmesh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@PawlikMichal25 @RodrigoSMarques @fischerscode