Opened 7 years ago

Last modified 3 years ago

#30 new task

Security review (ongoing)

Reported by: kdeugau Owned by:
Priority: major Milestone:
Version: Keywords:
Cc:

Description (last modified by kdeugau)

XSS/input validation:
Reading back on VegaDNS' history I poked into the CVE issues reported with VegaDNS 0.9.9.1 and 1.1.4. I realized the same message-reporting vulnerability would bite here.

Access scoping:
Check to make sure a user can't access any entity outside of their group tree

Change History (16)

comment:1 Changed 7 years ago by kdeugau

(In [173]) /trunk

Security audit (see #30)

  • remove a stale global
  • catch and handle bad page parameter

comment:2 Changed 7 years ago by kdeugau

(In [174]) /trunk

Security review (See #30)

  • convert error-message-passing via changepage to use the session to store the message so it can't be fiddled with in transit

comment:3 Changed 7 years ago by kdeugau

(In [176]) /trunk

Remove some more stale commented code
Remove redundant call to initialze $searchsubs
Security review (see #30)

  • set $webvar{page} a little earlier so we don't clutter the session with unusable data
  • tweak initialization of $searchsubs. Improved but will still behave a bit strangely if extra data is deliberately or accidentally added to $webvar{searchsubs} (see #31)

comment:4 Changed 7 years ago by kdeugau

(In [177]) /trunk

Security review (see #30)

  • convert resultmsg and warnmsg message-passing to use the session as a data store

comment:5 Changed 7 years ago by kdeugau

(In [178]) /trunk

Security review (See #30)

  • fix up ACL handling in group modification; as with user editing, the user may not make any change that includes access that user does not already have. This may mean removing a permission previously set but which the user doesn't have.

comment:6 Changed 7 years ago by kdeugau

  • Description modified (diff)
  • Summary changed from Security review - XSS/input validation to Security review

Change ticket info to cover all security-review changes

comment:7 Changed 7 years ago by kdeugau

(In [179]) /trunk

Fix up ACL and scope checks on groups (see #30)

  • check correct ACL permission for group creation
  • check parent group of a new group is in-scope for user
  • make sure new groups do not exceed the ACL given to the user
  • normalize action reporting (success and warning) compared to the same actions elsewhere
  • check scope deleting a group
  • check scope editing a group

comment:8 Changed 7 years ago by kdeugau

(In [180]) /trunk

Apply scope checks on user and domain for logs. Scope checks
for group logs are applied through the change-group check since
the group log is for the current group instead of user input.
See #30.

comment:9 Changed 7 years ago by kdeugau

(In [181]) /trunk

Scope check (see #30)

  • user add
  • user edit/update
  • user delete

comment:10 Changed 7 years ago by kdeugau

(In [181]) /trunk

Scope check (see #30)

  • user add
  • user edit/update
  • user delete

comment:11 Changed 7 years ago by kdeugau

(In [182]) /trunk

Scope checks (See #30)

  • user delete

comment:12 Changed 7 years ago by kdeugau

  • Summary changed from Security review to Security review (ongoing)
  • Type changed from defect to task

comment:13 Changed 6 years ago by kdeugau

(In [508]) /trunk

Add some extra fencing aroung the sortorder and sortby arguments to
various get*List methods to prevent SQL injection attacks. See #30.

comment:14 Changed 5 years ago by kdeugau

(In [591]) /trunk

  • Session-handling tweak; set cookies to expire so use of the Back button doesn't result in read-only access to everything.
  • Fiddle group handling in session data and on constructing URLs; side effects possibly related to the session issue caused a user in a subgroup to get mistakenly fed data for the root group - except for the group list in the menu. Arguably security fixes; see #30.

comment:15 Changed 5 years ago by kdeugau

(In [592]) /trunk

Review, fix, tweak, and fine-tune user permission add/update handling.

Confirm that it now correctly limits a user to granting only those
permissions they already hold, and only warns when the requested
permissions are really greater or mismatched.

Also catch a possible malicious escalation to superuser/admin status.

Confirm it *should* be impossible to generate this warning via normal
web UI actions; someone would have to manually construct a URL or POST
request with the appropriate fields.

See #30.

comment:16 Changed 3 years ago by kdeugau

(In [706]) /trunk

Security improvement; add -ip_match to use CGI::Session. User IPs
should not generally mutate over the course of a session, and if they
do, chances are other things will be breaking all over the place too.
See #30.

Note: See TracTickets for help on using tickets.