[Issue #12] Security Audit #26

SammySteiner · 2024-06-07T17:09:33Z

Ticket

Resolves Audit application security #12

Changes

Added application-security.md file in the docs/app-rails folder based on this railsdoc https://guides.rubyonrails.org/v7.1/security.html.

Context for reviewers

Please read through the file and give feedback if the content is clear and concise.
Note there are 16 TODO items, please give feedback if any of those need clarification or discussion.
I'm not super familiar with Rails and may have completely miss understood some of the recommendations as well as how we implemented things in the template, please let me know if you this I may have misunderstood something.

rocketnova

@SammySteiner My high level feedback is related to the template nature of this repo. The audience for this document should be engineers who are working on an application based on this template, not engineers working on the template (i.e. us).

I suggest that you re-organize this document into a checklist of best practices that the template is currently doing, a checklist of best practices that the template is not yet doing. For anything that could be added in the future (i.e. not a one-time configuration setting, but something to check again if the application develops), add a note that calls that out.

For example, something like this:

SSL (config.force_ssl = true) is enforced in production environments.
CSRF protection is enabled in production environments.
Expire sessions after a set amount of time, regardless of activity.
- NOTE: This should be enforced by the auth service, such as by AWS Cognito.
For all forms accessible by non-authenticated users, configure honeypot fields and logic to thwart bots.
- ONGOING NOTE: Check this anytime a new form is added to the application.

docs/app-rails/application-security.md

rocketnova · 2024-06-07T18:02:58Z

docs/app-rails/application-security.md

+1. Change password includes email 6 digit code.
+1. **TODO:** Require entering the password when changing email.
+#### 5.3 CAPTCHA
+1. **TODO:** Include Captcha on account creation, login, change password, and change email.


Let's make this its own ticket. I think we may choose not to implement this one based on UX best practices that might contradict this.

I'll make this an item to consider with thought towards UX and callout the accessibility issues.

rocketnova · 2024-06-07T18:07:35Z

docs/app-rails/application-security.md

+1. **TODO:** Require entering the password when changing email.
+#### 5.3 CAPTCHA
+1. **TODO:** Include Captcha on account creation, login, change password, and change email.
+1. **TODO:** Include honeypot fields and logic on Non logged in forms to catch bots that spam all fields (good resource: https://nedbatchelder.com/text/stopbots.html).


Do you feel it's necessary to include honeypots for the template? I think the TODO here might be to include it as a recommended best practice and say something like:

**NOTE:** Any forms added to the application that are accessible by unauthenticated users should refer to the [Rails Guide on Security, section 5.3](https://guides.rubyonrails.org/security.html#captchas).

In the updated format I left it as an open checkbox. It seems like at minimum, these honeypot fields are hidden from users viewing the application on a screen. However, we should ensure that users with screen readers are informed not to fill in the honeypot field.

rocketnova · 2024-06-07T18:08:45Z

docs/app-rails/application-security.md

+1. Two places where regex is used (`mailto.rb` and devise) use the correct ruby `\A` and `\z` and not the more common: `/^` and `$/`.
+1. **TODO:** Add multiline: true to our regex format: in validations.
+#### 5.6 Privilege Escalation
+1. **TODO:** By manually changing a parameter, a hacker may get access to information they should not be able to access. Instead of doing things like: `@task = Task.find(params[:id])`, instead do `@user.tasks.find(params[:id])`. This is not in the template, but it is in the app the template is based on.


This is a good catch, but does not belong in this document because this is for the template. Please create an issue for this in the PFML Accelerator repo.

I looked into this a bit more. In general the application is using a library, pundit, to wire up resource authorization and is calling the authorize method when looking up models. It seems like a good best practice to look up the model from and id based on the user, but it may be redundant to create tickets for that if we're already using Pundit. Thoughts?

In the specific case I found, it's in the Admin view, so I'm guessing that should be allowed. However, it should also add the authorize method it's using elsewhere in case we either don't want admins to be able to see any data from any user, or if we want to have different types of admins with different levels of permissions. But maybe that doesn't need to happen at this point?

Co-authored-by: Rocket <[email protected]>

SammySteiner · 2024-06-10T13:35:46Z

@SammySteiner My high level feedback is related to the template nature of this repo. The audience for this document should be engineers who are working on an application based on this template, not engineers working on the template (i.e. us).

I suggest that you re-organize this document into a checklist of best practices that the template is currently doing, a checklist of best practices that the template is not yet doing. For anything that could be added in the future (i.e. not a one-time configuration setting, but something to check again if the application develops), add a note that calls that out.

For example, something like this:

SSL (config.force_ssl = true) is enforced in production environments.

CSRF protection is enabled in production environments.

Expire sessions after a set amount of time, regardless of activity.

NOTE: This should be enforced by the auth service, such as by AWS Cognito.

For all forms accessible by non-authenticated users, configure honeypot fields and logic to thwart bots.

ONGOING NOTE: Check this anytime a new form is added to the application.

Thanks @rocketnova! This is the guidance I needed to take this doc from personal notes as I was reading through the documentation to something that's useful and digestible for other people.

docs/app-rails/application-security.md

rocketnova

@SammySteiner Thanks for reworking this file! See below for questions and suggestions for clarity.

docs/app-rails/application-security.md

rocketnova · 2024-06-17T22:26:10Z

docs/app-rails/application-security.md

+- [x] GET, POST, DELETE, and rails’ resources are used appropriately in the `routes.rb` file.
+- [x] Pass a CSRF token to the client.
+    - Note: This is accomplished with `<%= csrf_meta_tags %>` in [application.html.erb](app-rails/app/views/layouts/application.html.erb)
+- [ ] Set forgery protection in production  


Is this something we can set in the template?

Can you go ahead and open the follow-up ticket(s) and list which items we are going to pursue?

docs/app-rails/application-security.md

rocketnova · 2024-06-17T22:57:09Z

docs/app-rails/application-security.md

+- [x] Don't use the `open()` method to access files, instead use `File.open()` or `IO.open()` that will not execute commands. 
+- [ ] User inputs that are added to the document header and appear in the `<head></head>` tags in erb files, use the `content_for :head` method in erb files, and adding user input to a response with `head :some_header` in controllers. 
+    - Note: The template doesn't use any of those features, but this may come up in the future.
+- [ ] [`ActionDispatch::HostAuthorization`](https://guides.rubyonrails.org/configuring.html#actiondispatch-hostauthorization) is configured in production to prevent DNS rebinding attacks.


Is this one we can implement in the template?

Yes, we'd use the APP_HOST env variable to match part of the URL.

rocketnova · 2024-06-17T22:57:28Z

docs/app-rails/application-security.md

+    * `spawn()`
+    * `command`
+- [x] Don't use the `open()` method to access files, instead use `File.open()` or `IO.open()` that will not execute commands. 
+- [ ] User inputs that are added to the document header and appear in the `<head></head>` tags in erb files, use the `content_for :head` method in erb files, and adding user input to a response with `head :some_header` in controllers. 


I think this line needs re-writing. I don't understand it.

It's referring to user inputs that are interpolated into the header element in the HTML doc via .erb files or into the response header from a controller. These methods help sanitize those strings. However, I can't find where that guidance was from. I updated the doc, but we could also cut this recommendation too if you think it's not necessary.

My two cents would be to cut it. I think it's a good general recommendation, but none of our common use case projects accept user input in this way (e.g. building a CMS).

docs/app-rails/application-security.md

rocketnova · 2024-06-17T22:59:13Z

docs/app-rails/application-security.md

+- [x] Use a permitted list of tags in inputs that allow html or when allowing a text input that will be converted into html, using:
+    - Note: The most common Rails tool for text to html conversion is RedCloth.
+```
+tags = %w(a acronym b strong i em li ul ol h1 h2 h3 h4 h5 h6 blockquote br cite sub sup ins p)
+s = sanitize(user_input, tags: tags, attributes: %w(href title))
+```


I think you might be able to indent the code fenced block with 4 spaces, so that it doesn't break up the unordered list.

That worked! Thank you.

Co-authored-by: Rocket <[email protected]>

rocketnova

Excellent! Thank you!

SammySteiner · 2024-06-25T15:19:46Z

@rocketnova
Created Tickets:

SammySteiner added 5 commits June 6, 2024 17:44

app security doc and items 1-6

423d20d

completed injection section

5691c78

item 7

a1e7bac

section 8

cf679e3

complete audit

9cbec94

SammySteiner requested review from rocketnova, lorenyu and ellery-nava June 7, 2024 17:09

Merge branch 'main' into sammysteiner/12-security-audit

67220ef

rocketnova reviewed Jun 7, 2024

View reviewed changes

SammySteiner and others added 7 commits June 10, 2024 09:20

Update docs/app-rails/application-security.md

5a6c050

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

cdbc423

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

2b317e8

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

37ea49b

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

1e509b7

Co-authored-by: Rocket <[email protected]>

remove suggestion to add todos to the codebase

41451e3

Remove unnecessary headings

703b7bb

SammySteiner added 10 commits June 10, 2024 14:56

Merge branch 'main' into sammysteiner/12-security-audit

f7c91b0

checkboxes

ade9852

CSRF section

1107398

Redirection and Files section

61b7923

User Management section

c25f99c

Unsafe Query Generation

eecc240

HTTP Security Headers section

24acba2

Admin section

ad4bf15

Injection section

00f2c78

authorization library

11be9c5

SammySteiner commented Jun 11, 2024

View reviewed changes

docs/app-rails/application-security.md Outdated Show resolved Hide resolved

Merge branch 'main' into sammysteiner/12-security-audit

57d2faf

rocketnova reviewed Jun 17, 2024

View reviewed changes

SammySteiner and others added 13 commits June 18, 2024 15:25

Update docs/app-rails/application-security.md

5cad12f

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

ac26ad0

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

eb76cc2

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

fc9fd5a

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

bcf2472

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

44fb76b

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

6502eab

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

37a847e

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

5e9644a

Co-authored-by: Rocket <[email protected]>

Update docs/app-rails/application-security.md

e342374

Co-authored-by: Rocket <[email protected]>

Merge branch 'main' into sammysteiner/12-security-audit

5431222

indent code block

5087f38

header injection solution clarification

a92c34c

rocketnova approved these changes Jun 18, 2024

View reviewed changes

SammySteiner added 2 commits June 25, 2024 09:44

Remove header interpolation guidance

7364aa1

Merge branch 'main' into sammysteiner/12-security-audit

d5e82f9

SammySteiner merged commit 4474a89 into main Jun 25, 2024
3 checks passed

SammySteiner deleted the sammysteiner/12-security-audit branch June 25, 2024 15:20

[Issue #12] Security Audit #26

[Issue #12] Security Audit #26

Uh oh!

Conversation

SammySteiner commented Jun 7, 2024

Ticket

Changes

Context for reviewers

Uh oh!

rocketnova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SammySteiner commented Jun 10, 2024

Uh oh!

Uh oh!

rocketnova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocketnova left a comment

Choose a reason for hiding this comment

Uh oh!

SammySteiner commented Jun 25, 2024

Uh oh!

Uh oh!

Uh oh!