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

Using /sessions endpoint not creating restricted session tokens #6612

Closed
stevestencil opened this issue Apr 14, 2020 · 18 comments
Closed

Using /sessions endpoint not creating restricted session tokens #6612

stevestencil opened this issue Apr 14, 2020 · 18 comments
Labels
type:question Support or code-level question

Comments

@stevestencil
Copy link
Contributor

stevestencil commented Apr 14, 2020

Issue Description

When using the REST api to create a new session token as documented here the tokens are not set as restricted.

Steps to reproduce

Follow the instructions in the REST API Guide to create a restricted session token

Expected Results

A new session token should be created with restricted = true

Actual Outcome

A new session token is created with restricted = false

Versions

Parse-Server - 4.2.0
Parse-SDK-JS - 2.13.0

@EricNetsch
Copy link

@flovilmart @dplewis Yes, this is a major issue here among other related issues I will not disclose here

@EricNetsch
Copy link

@stevestencil Let us know which version of parse-server you are running?

@stevestencil
Copy link
Contributor Author

@EricNetsch I updated my description with the versions

@stale
Copy link

stale bot commented Jul 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 11, 2020
@davimacedo davimacedo added type:bug Impaired feature or lacking behavior that is likely assumed and removed wontfix labels Jul 13, 2020
@davimacedo
Copy link
Member

Hi guys. I've searched all the code base and also this issue and I believe that the restricted flag existed in parse.com but was never implemented in the Parse Server open source. It means that setting restricted to true or false has no effect. And that's why the guide steps are also not working. So, we have three options here:
1 - remove this feature from docs
2 - implement the feature
3 - 1, then 2

@dplewis @TomWFox @mtrezza thoughts?

@davimacedo davimacedo added discussion type:docs Only change in the docs or README type:feature New feature or improvement of existing feature and removed type:bug Impaired feature or lacking behavior that is likely assumed labels Jul 23, 2020
@mtrezza
Copy link
Member

mtrezza commented Jul 23, 2020

This issue and the linked issue seem to be a vulnerability disclosure, should we redact it?

I think it makes sense to remove the feature of restricted sessions, because:

  • From the docs I understand that restricted sessions is mainly a Parse IoT feature. The Parse Arduino SDK is retired and the Embedded C SDK shows limited activity, last relevant activity in 2016. There seem to be no active contributors nor experts for Parse IoT around. Some authors expressed interest in maintaining the SDK years ago, but it's questionable whether one maintainer would even be enough to keep it up-to-date. The danger with keeping this not-maintained SDK that is apparently not much in use either is what we are facing now: a fundamental security flaw that stays unnoticed for years.

  • It seems to be a half-baked feature, maybe even obsolete considering the advancements of ACL, because a restricted session can still read in User, Role and Session (except for unrestricted sessions), and can read/write data in any other class just like a normal session. Even the docs are somewhat contradictory:

Restricted Sessions are useful for “Parse for IoT” devices (e.g Arduino or Embedded C) that may run in a less-trusted physical environment than mobile apps. However, please keep in mind that restricted sessions can still read data on User, Session, and Role classes, and can read/write data in any other class just like a normal session. So it is still important for IoT devices to be in a safe physical environment and ideally use encrypted storage to store the session token.

Use restricted sessions in a "less-trusted physical environment" but use them in a "safe physical environment" doesn't make much sense to me. The more effective approach seems to be to store tokens on the IoT device in a secure storage and recommend a distinct user/role for the IoT.

Unless there is another use case (maybe we can hear from someone using the IoT SDK?) I would go for:

  • Remove the feature from code
  • Remove the feature from the docs and recommend to store the session token in a secure storage and use distinct users/roles for IoT
  • Retire Embedded C SDK and maybe start a call to actively look for maintainers on the Parse Server issue site for better visibility

@davimacedo
Copy link
Member

This issue and the linked issue seem to be a vulnerability disclosure, should we redact it?

Technically yes, but, since the other issue is public for 2 years, I believe that it is better to keep it public for reference and we use this current thread to discuss the future of the feature.

@ggkitsas
Copy link

Hi,

I am contacting you on behalf of Snyk Security team. We would like to ask if you have a timeline for fixing this issue.

Best,
Snyk Security Team

@mtrezza mtrezza removed the discussion label Nov 1, 2020
@TomWFox
Copy link
Contributor

TomWFox commented Nov 12, 2020

I was just looking at this new Snyk open source health report. Unfortunately we're being pulled down due to a couple outstanding vulnerabilities, this being one of them.

So, we have three options here:
1 - remove this feature from docs
2 - implement the feature
3 - 1, then 2

I think we should remove the feature from the docs immediately, and then remove the feature from the codebase asap so we can close this one out.

@TomWFox TomWFox added type:bug Impaired feature or lacking behavior that is likely assumed S3 and removed type:feature New feature or improvement of existing feature labels May 10, 2021
@snoopysecurity
Copy link

hi, this issue has been fixed right? i see multiple PRs being referenced

@mtrezza
Copy link
Member

mtrezza commented Sep 3, 2021

@snoopysecurity Thanks for bringing this up. I will look into whether this has been fixed and whether this is even an issue.

@mtrezza
Copy link
Member

mtrezza commented Sep 3, 2021

@snoopysecurity This has never been a vulnerability of this repository. The restricted session property is a mere code artifact of a feature that was never part of this repository. Parse Server has been open sourced from Parse.com in 2017; the feature was not part of the open source version, but the session property was not removed, apparently by mistake.

I went back to the initial Parse Server 2.0.0 release and I did not find any logic depending on the restricted field. The first PR that removed part of that code artifact was #4574. The rest of the artifact will be removed with #7543.

The Snky vulnerability Improper Authorization can therefore be removed for all versions of this repository.

@mtrezza mtrezza closed this as completed Sep 3, 2021
@mtrezza mtrezza reopened this Sep 3, 2021
@parse-community parse-community deleted a comment from github-actions bot Sep 3, 2021
@TomWFox
Copy link
Contributor

TomWFox commented Sep 4, 2021

@ggkitsas Could you remove/resolve this vulnerability as per the explanation above?

@mtrezza
Copy link
Member

mtrezza commented Sep 7, 2021

I contacted Snyk about this; it seems that only Snky picked this up by mistake an added it to their internal list.

@benjifin
Copy link

benjifin commented Sep 8, 2021

Hey @mtrezza - thanks for the clarifications and sorry for the inconvenience, we (Snyk) will revoke this and that should take affect after tomorrow.

@mtrezza
Copy link
Member

mtrezza commented Sep 10, 2021

Thanks @benjifin, it seems that it should have been "deactivated" by setting the versions <0.0.0. However, it still appears for the parse-server 4.10.3 package as an active vulnerability:

image

https://snyk.io/test/npm/parse-server/4.10.3

@benjifin
Copy link

benjifin commented Sep 10, 2021 via email

@mtrezza mtrezza added type:question Support or code-level question and removed type:docs Only change in the docs or README type:bug Impaired feature or lacking behavior that is likely assumed severity:medium labels Oct 18, 2021
@mtrezza
Copy link
Member

mtrezza commented Oct 18, 2021

Closing, as the issue is not a bug.

@mtrezza mtrezza closed this as completed Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:question Support or code-level question
Projects
None yet
Development

No branches or pull requests

8 participants