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

File extension whitelists for end users #3633

Merged
merged 32 commits into from
Apr 11, 2020
Merged

File extension whitelists for end users #3633

merged 32 commits into from
Apr 11, 2020

Conversation

donker
Copy link
Contributor

@donker donker commented Mar 18, 2020

Currently DNN uses one single file extension whitelist that governs what administrators and end users can upload (AE: Security > More > More Security Settings). This list includes files that may be needed by administrators to upload content to manage the site but that end users have no business uploading. This PR introduces a new whitelist mechanism that allows site maintainers (i.e. admins) to control what end users of a site can upload (within the limits of their own whitelist). Specifically this solution does the following:

  1. It introduces a second whitelist setting (AE: Security > More > More Security Settings) for the host called "Default End User Extension Whitelist". If an administrator does not define an end user whitelist, this list will be used.

  2. It allows the administrator to define a whitelist (AE: Site Settings > Site Behavior > More) called "Permitted File Extension List". The UI includes a dropdown which allows the admin to specify "default" (i.e. what is defined in 1 above), "custom", or "images only".

  3. Upon update of any of the above settings and the global whitelist, there is a cascading check to ensure no end user whitelist allows uploading of a file not allowed by the global whitelist.

Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

partial review

DNN Platform/Library/Data/DataProvider.cs Outdated Show resolved Hide resolved
DNN Platform/Library/Data/DataProvider.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

Partial Review

DNN Platform/Library/Data/DataProvider.cs Outdated Show resolved Hide resolved
/// <summary>
/// Gets whether the user is in the portal's administrators role
/// </summary>
private bool? isAdmin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the calculated value is never stored/used, no need for this to be a private backer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I am on the same page, but I am pretty confident that once we have this value then extension developers will begin using this value more and more instead of coding their own IsAdmin logic. And that might require accessing the value multiple times in your code.

On the other hand one could argue that maybe an extension developer might change the user's roles and then recalculation would be necessary.

On the fence about it. Given that the calculation is not trivial I persisted the value.

///
/// The only time we should call this is if the host allowed extensions list has changed
/// </summary>
public void CheckAllPortalFileExtensionWhitelists()
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is internal, can we mark this internal to prevent external callers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally I think we might want a bit stronger name on this, as this isn't a check, it actually updates/changes data if necessary

@bdukes
Copy link
Contributor

bdukes commented Apr 9, 2020

Looks like tests are failing. I only glanced at the error, but you may need to add a mock value for this new setting or something like that.

@valadas
Copy link
Contributor

valadas commented Apr 10, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@mitchelsellers mitchelsellers merged commit 8fb142d into dnnsoftware:develop Apr 11, 2020
@donker donker deleted the whitelist branch April 12, 2020 12:11
@madneal
Copy link

madneal commented Apr 15, 2020

Is this fix for CVE-2020-5186. Why there is no release description for security fix?

@mitchelsellers
Copy link
Contributor

@neal1991 Security issues ARE NOT DISCUSSED here per the terms of our security policy.

If this is applicable to a security notice it will be published as part of the Security Bulletins as part of the release process.

We DO NOT publish security bulletins for RC releases.

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

Successfully merging this pull request may close these issues.

6 participants