#8266: Security sensitive permissions (Lombiq Technologies: ORCH-214) (#8874)

* Adding IsSecurityCritical property to Permission class and setting up Security Critical permissions

* Setting up default hint for security critical permissions

* Adding GetSecurityCriticalPermissions to IRoleService

* Indicating security critical permissions in the role editor

* Indicating roles that have security sensitive permissions in the UserRolesPart editor

* Indicating roles that have security sensitive permissions in the Role Admin list

* Code styling

* Styling

* Removing empty line

* Code styling in Orchard.Roles/Views/Admin/Index.cshtml

* Optimizing and standardizing the processing of security critical permissions / roles + code styling

* Simplifying the GetSecurityCriticalPermissions method

Co-authored-by: Benedek Farkas <benedek.farkas@lombiq.com>

* Removing setting up a Hint for security critical permissions

* Adding a meaningful summary for the IsSecurityCritical property of Permission

* Removing the Hint property from the Permission

---------

Co-authored-by: Benedek Farkas <benedek.farkas@lombiq.com>

src/Orchard.Web/Core/Settings/Permissions.cs

@@ -6,7 +6,7 @@ namespace Orchard.Core.Settings
 {
     public class Permissions : IPermissionProvider
     {
-        public static readonly Permission ManageSettings = new Permission { Description = "Manage Settings", Name = "ManageSettings" };
+        public static readonly Permission ManageSettings = new Permission { Description = "Manage Settings", Name = "ManageSettings", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 
@@ -28,4 +28,4 @@ namespace Orchard.Core.Settings
         }
 
     }
-}
+}

src/Orchard.Web/Modules/Orchard.AuditTrail/Permissions.cs

@@ -6,10 +6,10 @@ namespace Orchard.AuditTrail
 {
     public class Permissions : IPermissionProvider
     {
-        public static readonly Permission ViewAuditTrail = new Permission { Description = "View audit trail", Name = "ViewAuditTrail" };
-        public static readonly Permission ManageAuditTrailSettings = new Permission { Description = "Manage audit trail settings", Name = "ManageAuditTrailSettings" };
-        public static readonly Permission ImportAuditTrail = new Permission { Description = "Import audit trail", Name = "ImportAuditTrail" };
-        public static readonly Permission ManageClientIpAddressSettings = new Permission { Description = "Manage client IP address settings", Name = "ManageClientIpAddressSettings" };
+        public static readonly Permission ViewAuditTrail = new Permission { Description = "View audit trail", Name = "ViewAuditTrail", IsSecurityCritical = true };
+        public static readonly Permission ManageAuditTrailSettings = new Permission { Description = "Manage audit trail settings", Name = "ManageAuditTrailSettings", IsSecurityCritical = true };
+        public static readonly Permission ImportAuditTrail = new Permission { Description = "Import audit trail", Name = "ImportAuditTrail", IsSecurityCritical = true };
+        public static readonly Permission ManageClientIpAddressSettings = new Permission { Description = "Manage client IP address settings", Name = "ManageClientIpAddressSettings", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 
@@ -35,4 +35,4 @@ namespace Orchard.AuditTrail
             };
         }
     }
-}
+}

src/Orchard.Web/Modules/Orchard.ContentTypes/Permissions.cs

@@ -7,7 +7,7 @@ namespace Orchard.ContentTypes
     public class Permissions : IPermissionProvider
     {
         public static readonly Permission ViewContentTypes = new Permission { Name = "ViewContentTypes", Description = "View content types" };
-        public static readonly Permission EditContentTypes = new Permission { Name = "EditContentTypes", Description = "Edit content types" };
+        public static readonly Permission EditContentTypes = new Permission { Name = "EditContentTypes", Description = "Edit content types", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 

src/Orchard.Web/Modules/Orchard.ImportExport/Permissions.cs

@@ -6,8 +6,8 @@ namespace Orchard.ImportExport
 {
     public class Permissions : IPermissionProvider
     {
-        public static readonly Permission Import = new Permission { Description = "Import Data", Name = "Import" };
-        public static readonly Permission Export = new Permission { Description = "Export Data", Name = "Export" };
+        public static readonly Permission Import = new Permission { Description = "Import Data", Name = "Import", IsSecurityCritical = true };
+        public static readonly Permission Export = new Permission { Description = "Export Data", Name = "Export", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 
@@ -26,4 +26,4 @@ namespace Orchard.ImportExport
                          };
         }
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Modules/Permissions.cs

@@ -6,7 +6,7 @@ namespace Orchard.Modules
 {
     public class Permissions : IPermissionProvider
     {
-        public static readonly Permission ManageFeatures = new Permission { Description = "Manage Features", Name = "ManageFeatures" };
+        public static readonly Permission ManageFeatures = new Permission { Description = "Manage Features", Name = "ManageFeatures", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 
@@ -25,4 +25,4 @@ namespace Orchard.Modules
                          };
         }
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Packaging/Permissions.cs

@@ -6,7 +6,7 @@ namespace Orchard.Packaging
 {
     public class Permissions : IPermissionProvider
     {
-        public static readonly Permission ManagePackages = new Permission { Description = "Manage packages", Name = "ManagePackages" };
+        public static readonly Permission ManagePackages = new Permission { Description = "Manage packages", Name = "ManagePackages", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 
@@ -21,4 +21,4 @@ namespace Orchard.Packaging
             return new List<PermissionStereotype>();
         }
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Roles/Controllers/AdminController.cs

@@ -65,7 +65,16 @@ namespace Orchard.Roles.Controllers
             if (!Services.Authorizer.Authorize(Permissions.ManageRoles, T("Not authorized to manage roles")))
                 return new HttpUnauthorizedResult();
 
-            var model = new RolesIndexViewModel { Rows = _roleService.GetRoles().OrderBy(r => r.Name).ToList() };
+            var securityCriticalPermissions = _roleService.GetSecurityCriticalPermissions().ToHashSet();
+            var roles = _roleService.GetRoles().OrderBy(r => r.Name).ToList();
+
+            var model = new RolesIndexViewModel
+            {
+                Rows = roles,
+                RolesWithSecurityCriticalPermissions = roles.ToDictionary(
+                    role => role.Name,
+                    role => role.RolesPermissions.Any(p => securityCriticalPermissions.Contains(p.Permission.Name)))
+            };
 
             return View(model);
         }
@@ -250,18 +259,21 @@ namespace Orchard.Roles.Controllers
             {
                 return new HttpUnauthorizedResult();
             }
+
+            var securityCriticalPermissions = _roleService.GetSecurityCriticalPermissions().ToHashSet();
             // create the ViewModel used to manage a user's roles
             var model = new UserRolesViewModel
             {
                 User = userRolesPart.As<IUser>(),
                 UserRoles = userRolesPart,
-                Roles = allRoles.Select(x => new UserRoleEntry
+                Roles = allRoles.Select(role => new UserRoleEntry
                 {
-                    RoleId = x.Id,
-                    Name = x.Name,
-                    Granted = userRolesPart.Roles.Contains(x.Name)
+                    RoleId = role.Id,
+                    Name = role.Name,
+                    Granted = userRolesPart.Roles.Contains(role.Name),
+                    HasSecurityCriticalPermissions = role.RolesPermissions.Any(p => securityCriticalPermissions.Contains(p.Permission.Name))
                 }).ToList(),
-                AuthorizedRoleIds = authorizedRoleIds
+                AuthorizedRoleIds = authorizedRoleIds,
             };
 
             // this calls the same view used by the driver that lets users with higher

src/Orchard.Web/Modules/Orchard.Roles/Drivers/UserRolesPartDriver.cs

@@ -67,20 +67,22 @@ namespace Orchard.Roles.Drivers
                 {
                     return null;
                 }
-                var allRoles = _allRoles.Value
-                    .Select(x => new UserRoleEntry
-                    {
-                        RoleId = x.Id,
-                        Name = x.Name,
-                        Granted = userRolesPart.Roles.Contains(x.Name)
-                    });
+
+                var securityCriticalPermissions = _roleService.GetSecurityCriticalPermissions().ToHashSet();
                 var model = new UserRolesViewModel
                 {
                     User = userRolesPart.As<IUser>(),
                     UserRoles = userRolesPart,
-                    Roles = allRoles.ToList(),
-                    AuthorizedRoleIds = authorizedRoleIds
+                    Roles = _allRoles.Value.Select(role => new UserRoleEntry
+                    {
+                        RoleId = role.Id,
+                        Name = role.Name,
+                        Granted = userRolesPart.Roles.Contains(role.Name),
+                        HasSecurityCriticalPermissions = role.RolesPermissions.Any(p => securityCriticalPermissions.Contains(p.Permission.Name))
+                    }).ToList(),
+                    AuthorizedRoleIds = authorizedRoleIds,
                 };
+
                 return shapeHelper.EditorTemplate(TemplateName: TemplateName, Model: model, Prefix: Prefix);
             });
 

src/Orchard.Web/Modules/Orchard.Roles/Permissions.cs

@@ -12,8 +12,8 @@ namespace Orchard.Roles
     {
         private readonly IRepository<RoleRecord> _roleRepository;
 
-        public static readonly Permission ManageRoles = new Permission { Description = "Managing Roles", Name = "ManageRoles" };
-        public static readonly Permission AssignRoles = new Permission { Description = "Assign Roles", Name = "AssignRoles", ImpliedBy = new[] { ManageRoles } };
+        public static readonly Permission ManageRoles = new Permission { Description = "Managing Roles", Name = "ManageRoles", IsSecurityCritical = true, };
+        public static readonly Permission AssignRoles = new Permission { Description = "Assign Roles", Name = "AssignRoles", IsSecurityCritical = true, ImpliedBy = new[] { ManageRoles } };
 
         public virtual Feature Feature { get; set; }
 
@@ -80,4 +80,4 @@ namespace Orchard.Roles
         }
 
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Roles/Services/IRoleService.cs

@@ -14,6 +14,7 @@ namespace Orchard.Roles.Services
         void UpdateRole(int id, string roleName, IEnumerable<string> rolePermissions);
         void DeleteRole(int id);
         IDictionary<string, IEnumerable<Permission>> GetInstalledPermissions();
+        IEnumerable<string> GetSecurityCriticalPermissions();
         IEnumerable<string> GetPermissionsForRole(int id);
 
         IEnumerable<string> GetPermissionsForRoleByName(string name);
@@ -26,4 +27,4 @@ namespace Orchard.Roles.Services
         /// <returns>Returns false if a role with the given name already exits</returns>
         bool VerifyRoleUnicity(string name);
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Roles/Services/RoleService.cs

@@ -203,6 +203,9 @@ namespace Orchard.Roles.Services
             return installedPermissions;
         }
 
+        public IEnumerable<string> GetSecurityCriticalPermissions() =>
+            _permissionProviders.SelectMany(pp => pp.GetPermissions().Where(p => p.IsSecurityCritical)).Select(p => p.Name);
+
         public IEnumerable<string> GetPermissionsForRole(int id)
         {
             var permissions = new List<string>();
@@ -251,4 +254,4 @@ namespace Orchard.Roles.Services
             _signals.Trigger(SignalName);
         }
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Roles/ViewModels/RolesIndexViewModel.cs

@@ -6,5 +6,7 @@ namespace Orchard.Roles.ViewModels
     public class RolesIndexViewModel
     {
         public IList<RoleRecord> Rows { get; set; }
+
+        public Dictionary<string, bool> RolesWithSecurityCriticalPermissions { get; set; }
     }
 }

src/Orchard.Web/Modules/Orchard.Roles/ViewModels/UserRolesViewModel.cs

@@ -23,5 +23,6 @@ namespace Orchard.Roles.ViewModels
         public int RoleId { get; set; }
         public string Name { get; set; }
         public bool Granted { get; set; }
+        public bool HasSecurityCriticalPermissions { get; set; }
     }
 }

src/Orchard.Web/Modules/Orchard.Roles/Views/Admin/Edit.cshtml

@@ -1,67 +1,93 @@
+@using Orchard.Roles.ViewModels
+
 @model RoleEditViewModel
-@using Orchard.Roles.ViewModels;
 
-@{ Layout.Title = T("Edit Role").ToString(); }
+@{
+    Style.Require("FontAwesome");
+    Layout.Title = T("Edit Role").ToString();
+}
 
-@using(Html.BeginFormAntiForgeryPost()) {
+@using (Html.BeginFormAntiForgeryPost())
+{
     @Html.ValidationSummary();
     <fieldset>
         <legend>@T("Information")</legend>
         <label for="pageTitle">@T("Role Name:")</label>
-        @if (Model.Name == "Administrator") { // the one special case
+        @if (Model.Name == "Administrator")
+        { // the one special case
             <input id="Name" class="text" type="text" value="@Model.Name" readonly="readonly" />
             <input class="text" name="Name" type="hidden" value="@Model.Name" />
         }
-        else {
+        else
+        {
             <input id="Name" class="text" name="Name" type="text" value="@Model.Name" />
         }
         <input type="hidden" value="@Model.Id" name="Id" />
     </fieldset>
     <fieldset>
         <legend>@T("Permissions")</legend>
-        @foreach (var category in Model.RoleCategoryPermissions.Keys) {
-        <fieldset>
-            <legend>@category</legend>
-            <table class="items">
-                <colgroup>
-                    <col id="Col1" />
-                    <col id="Col2" />
-                </colgroup>
-                <thead>
-                    <tr>
-                        <th scope="col">@T("Permission")</th>
-                        <th scope="col">@T("Allow")</th>
-                        <th scope="col">@T("Effective")</th>
-                    </tr>
-                </thead>
-                @foreach (var permission in Model.RoleCategoryPermissions[category]) {
-                <tr>
-                    <td>@T(permission.Description)</td>
-                    <td style="width:60px;">
-                        @if (Model.CurrentPermissions.Contains(permission.Name)) {
-                            <input type="checkbox" value="true" name="Checkbox.@permission.Name" checked="checked"/>
-                         } 
-                         else {
-                            <input type="checkbox" value="true" name="Checkbox.@permission.Name"/>
-                        }
-                    </td>    
-                    <td style="width:60px;">
-                    @if (Model.EffectivePermissions.Contains(permission.Name)) {
-                            <input type="checkbox" disabled="disabled" name="Effective.@permission.Name" checked="checked"/>
-                     } else {
-                            <input type="checkbox" disabled="disabled" name="Effective.@permission.Name"/>
-                        }
-                    </td>                
-                </tr>
-                }
-            </table>
+
+        <p>@T("Allow: Permission is granted explicitly.")</p>
+        <p>@T("Effective: Permission is implied by a higher level permission, or explicitly granted.")</p>
+
+        <br />
+
+        @foreach (var category in Model.RoleCategoryPermissions.Keys)
+        {
+            <fieldset>
+                <legend>@category</legend>
+                <table class="items">
+                    <colgroup>
+                        <col id="Col1" />
+                        <col id="Col2" />
+                    </colgroup>
+                    <thead>
+                        <tr>
+                            <th scope="col">@T("Permission")</th>
+                            <th scope="col">@T("Allow")</th>
+                            <th scope="col">@T("Effective")</th>
+                        </tr>
+                    </thead>
+                    @foreach (var permission in Model.RoleCategoryPermissions[category])
+                    {
+                        <tr>
+                            <td>@T(permission.Description)</td>
+                            <td style="width:60px;">
+                                @if (Model.CurrentPermissions.Contains(permission.Name))
+                                {
+                                    <input type="checkbox" value="true" name="Checkbox.@permission.Name" checked="checked" />
+                                }
+                                else
+                                {
+                                    <input type="checkbox" value="true" name="Checkbox.@permission.Name" />
+                                }
+                                @if (permission.IsSecurityCritical)
+                                {
+                                    <i class="fa-solid fa-triangle-exclamation" style="color: red"
+                                       title="@T("This permission could allow a user to elevate their other permissions. Grant it with extreme consideration.")"></i>
+                                }
+                            </td>
+                            <td style="width:60px;">
+                                @if (Model.EffectivePermissions.Contains(permission.Name))
+                                {
+                                    <input type="checkbox" disabled="disabled" name="Effective.@permission.Name" checked="checked" />
+                                }
+                                else
+                                {
+                                    <input type="checkbox" disabled="disabled" name="Effective.@permission.Name" />
+                                }
+                            </td>
+                        </tr>
+                    }
+                </table>
             </fieldset>
-            }
+        }
     </fieldset>
     <fieldset>
         <button class="primaryAction" type="submit" name="submit.Save" value="@T("Save")">@T("Save")</button>
-        @if (Model.Name != "Administrator") {
-        <button type="submit" name="submit.Delete" value="@T("Delete")" itemprop="RemoveUrl">@T("Delete")</button>
+        @if (Model.Name != "Administrator")
+        {
+            <button type="submit" name="submit.Delete" value="@T("Delete")" itemprop="RemoveUrl">@T("Delete")</button>
         }
     </fieldset>
-}
+}

src/Orchard.Web/Modules/Orchard.Roles/Views/Admin/Index.cshtml

@@ -1,14 +1,17 @@
+@using Orchard.Roles.ViewModels
+
 @model RolesIndexViewModel
-@using Orchard.Roles.ViewModels;
-@using Orchard.Utility.Extensions;
+
 @{
     Script.Require("ShapesBase");
+    Style.Require("FontAwesome");
 
     Layout.Title = T("Roles").ToString();
 }
 
-@using (Html.BeginFormAntiForgeryPost()) {
-    @Html.ValidationSummary();
+@using (Html.BeginFormAntiForgeryPost())
+{
+    @Html.ValidationSummary()
     <fieldset class="bulk-actions">
         <label for="publishActions">@T("Actions:")</label>
         <select id="Select1" name="roleActions">
@@ -31,18 +34,28 @@
                     <th scope="col"></th>
                 </tr>
             </thead>
-            @foreach (var row in Model.Rows) {
+            @foreach (var row in Model.Rows)
+            {
                 <tr>
                     <td><input type="checkbox" value="true" name="@("Checkbox." + row.Id)" /></td>
-                    <td>@Html.ActionLink(row.Name, "Edit", new { row.Id })</td>
+                    <td>
+                        @Html.ActionLink(row.Name, "Edit", new { row.Id })
+                        @if (Model.RolesWithSecurityCriticalPermissions[row.Name])
+                        {
+                            <i class="fa-solid fa-triangle-exclamation" style="color: red"
+                                title="@T("This role contains security critical permissions.")"></i>
+                        }
+                    </td>
                     <td>
                         <ul class="action-links">
                             <li class="action-link">
                                 @Html.ActionLink(T("Edit").ToString(), "Edit", new { row.Id })
                             </li>
-                            @if (row.Name != "Administrator") {
+                            @if (row.Name != "Administrator")
+                            {
                                 <li class="action-link">
-                                    <a href="@Url.Action("Delete", new {row.Id, returnUrl = ViewContext.RequestContext.HttpContext.Request.RawUrl})" itemprop="RemoveUrl UnsafeUrl">@T("Delete")</a>                       
+                                    <a href="@Url.Action("Delete", new {row.Id, returnUrl = ViewContext.RequestContext.HttpContext.Request.RawUrl})"
+                                       itemprop="RemoveUrl UnsafeUrl">@T("Delete")</a>
                                 </li>
                             }
                         </ul>

src/Orchard.Web/Modules/Orchard.Roles/Views/EditorTemplates/Parts/Roles.UserRoles.cshtml

@@ -4,13 +4,20 @@
 
 @model UserRolesViewModel
 
+@{
+    Style.Require("FontAwesome");
+}
+
 <fieldset>
     <legend>@T("Roles")</legend>
 
-    @if (Model.Roles.Count > 0) {
+    @if (Model.Roles.Count > 0)
+    {
         var index = 0;
-        foreach (var entry in Model.Roles) {
-            if (SystemRoles.GetSystemRoles().Contains(entry.Name)) {
+        foreach (var entry in Model.Roles)
+        {
+            if (SystemRoles.GetSystemRoles().Contains(entry.Name))
+            {
                 continue;
             }
 
@@ -18,17 +25,26 @@
             @Html.Hidden("Roles[" + index + "].Name", entry.Name)
 
             <div>
-                @if (Model.AuthorizedRoleIds.Contains(entry.RoleId)) {
+                @if (Model.AuthorizedRoleIds.Contains(entry.RoleId))
+                {
                     @Html.CheckBox("Roles[" + index + "].Granted", entry.Granted)
-                } else {
+                }
+                else
+                {
                     @Html.CheckBox("Roles[" + index + "].Granted", entry.Granted, new { disabled = true })
                 }
                 <label class="forcheckbox" for="@Html.FieldIdFor(m => m.Roles[index].Granted)">@entry.Name</label>
+                @if (entry.HasSecurityCriticalPermissions)
+                {
+                    <i class="fa-solid fa-triangle-exclamation" style="color: red" title="@T("This role contains security critical permissions.")"></i>
+                }
             </div>
 
             index++;
         }
-    } else {
+    }
+    else
+    {
         <p>@T("There are no roles.")</p>
     }
 </fieldset>

src/Orchard.Web/Modules/Orchard.Templates/Permissions.cs

@@ -6,7 +6,7 @@ namespace Orchard.Templates
 {
     public class Permissions : IPermissionProvider
     {
-        public static readonly Permission ManageTemplates = new Permission { Description = "Managing Templates", Name = "ManageTemplates" };
+        public static readonly Permission ManageTemplates = new Permission { Description = "Managing Templates", Name = "ManageTemplates", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 
@@ -39,4 +39,4 @@ namespace Orchard.Templates
             };
         }
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Themes/Permissions.cs

@@ -6,7 +6,7 @@ namespace Orchard.Themes
 {
     public class Permissions : IPermissionProvider
     {
-        public static readonly Permission ApplyTheme = new Permission { Description = "Apply a Theme", Name = "ApplyTheme" };
+        public static readonly Permission ApplyTheme = new Permission { Description = "Apply a Theme", Name = "ApplyTheme", IsSecurityCritical = true };
 
         public virtual Feature Feature { get; set; }
 
@@ -27,4 +27,4 @@ namespace Orchard.Themes
                          };
         }
     }
-}
+}

src/Orchard.Web/Modules/Orchard.Users/Permissions.cs

@@ -7,7 +7,7 @@ namespace Orchard.Users
     public class Permissions : IPermissionProvider
     {
         public static readonly Permission ManageUsers =
-            new Permission { Description = "Managing Users", Name = "ManageUsers" };
+            new Permission { Description = "Managing Users", Name = "ManageUsers", IsSecurityCritical = true };
         public static readonly Permission ViewUsers =
             new Permission { Description = "View List of Users", Name = "ViewUsers", ImpliedBy = new[] { ManageUsers } };
 
@@ -31,4 +31,4 @@ namespace Orchard.Users
         }
 
     }
-}
+}

src/Orchard/Security/Permissions/Permission.cs

@@ -10,6 +10,11 @@ namespace Orchard.Security.Permissions
 
         public IEnumerable<Permission> ImpliedBy { get; set; }
 
+        /// <summary>
+        /// Indicates whether this permission could allow a user to elevate their other permissions.
+        /// </summary>
+        public bool IsSecurityCritical { get; set; }
+
         public static Permission Named(string name)
         {
             return new Permission { Name = name };

src/Orchard/Security/StandardPermissions.cs

@@ -10,7 +10,7 @@ namespace Orchard.Security
     {
         public static readonly Permission AccessAdminPanel = new Permission { Name = "AccessAdminPanel", Description = "Access admin panel" };
         public static readonly Permission AccessFrontEnd = new Permission { Name = "AccessFrontEnd", Description = "Access site front-end" };
-        public static readonly Permission SiteOwner = new Permission { Name = "SiteOwner", Description = "Site Owners Permission" };
+        public static readonly Permission SiteOwner = new Permission { Name = "SiteOwner", Description = "Site Owners Permission", IsSecurityCritical = true };
 
         public Feature Feature
         {
@@ -79,4 +79,4 @@ namespace Orchard.Security
         }
 
     }
-}
+}