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

docs: v1 secure coding guidelines #58

Merged
merged 46 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
66964df
initial draft of secure coding guidelines
sethkfman Oct 24, 2023
4423d0d
ran prettier
sethkfman Oct 24, 2023
94a0787
PR feedback updates on recommendations
sethkfman Oct 25, 2023
ff8b023
Clarify references used
Gudahtt Nov 2, 2023
dcf226b
Use headers for 6 guideline sections
Gudahtt Nov 2, 2023
4afebca
Use headers for guideline subsections
Gudahtt Nov 3, 2023
5c9f367
Spell out 3rd as Third
Gudahtt Nov 3, 2023
2154c1d
Expriment with numbered guidelines
Gudahtt Nov 3, 2023
a84e379
Drop guideline numbers
Gudahtt Nov 3, 2023
272b61c
Lint fixes
Gudahtt Nov 3, 2023
41fb22c
Clean up validation guidelines
Gudahtt Nov 5, 2023
1eb04da
Remove guideline about validation libraries
Gudahtt Nov 5, 2023
98bdca2
Expand dynamic code execution guideline
Gudahtt Nov 5, 2023
b86b880
Add output encoding guideline
Gudahtt Nov 5, 2023
9ac6a74
Add guideline about URI schemes
Gudahtt Nov 5, 2023
d2fbfe6
Remove guidelines about additional type checking
Gudahtt Nov 5, 2023
e6dcbd4
Minor change to wording
Gudahtt Nov 5, 2023
f49d9a0
Reword CSP guideline
Gudahtt Nov 5, 2023
01bc5a9
Fix data classification indentation
Gudahtt Nov 5, 2023
31907e5
Consolidate guidelines about embedding API tokens
Gudahtt Nov 5, 2023
000b006
Re-word guideline on sensitive data access
Gudahtt Nov 5, 2023
0cadbe7
Minor formatting changes
Gudahtt Nov 5, 2023
88a6b2a
Remove guideline about preventing console logging
Gudahtt Nov 5, 2023
ae5a14d
Remove detail on how security team recommendations are communicated; …
Gudahtt Nov 5, 2023
3e87345
Minor formatting change; remove trailing periods
Gudahtt Nov 5, 2023
567858b
Remove another guideline about disabling logging
Gudahtt Nov 5, 2023
ed1144f
Remove ambiguous error handling guideline
Gudahtt Nov 5, 2023
438ca29
Re-word error data leaking guideline, and add example
Gudahtt Nov 5, 2023
ff1eac2
Improve wording for entry about lockfiles/pinning dependencies
Gudahtt Nov 5, 2023
b30197b
Remove redundant dependabot guideline
Gudahtt Nov 5, 2023
704f165
Remove license guideline (not related to security)
Gudahtt Nov 5, 2023
e2a3cc5
Lint fix
Gudahtt Nov 5, 2023
43e5282
Expand upon dependency monitoring guideline
Gudahtt Nov 6, 2023
3793dd0
Add more detail on allow-scripts
Gudahtt Nov 6, 2023
62164d7
Expand on LavaMoat guideline
Gudahtt Nov 6, 2023
c4dc1d8
Remove guideline about accessing external data
Gudahtt Nov 6, 2023
3221913
Remove guideline about reducing privileges of API tokens
Gudahtt Nov 6, 2023
c4805ad
Add detail to guideline about using API keys securely
Gudahtt Nov 6, 2023
a684063
Split third party integrations from applications
Gudahtt Nov 6, 2023
6bf4c97
Remove auditability section
Gudahtt Nov 6, 2023
aad52de
Consolidate project secret guidelines
Gudahtt Nov 6, 2023
0162d18
Add guideline about preventing committing secrets
Gudahtt Nov 6, 2023
ece56ba
Improve wording
Gudahtt Nov 6, 2023
47d7ffb
Clarify that LavaMoat is JavaScript specific
Gudahtt Nov 6, 2023
3f7883e
Re-word dynamic code execution description to suggest asking security…
Gudahtt Nov 6, 2023
9716dc6
added doc to table of contents
sethkfman Nov 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ This is a living repository — nothing is set in stone! If you're member of Met
- [Guide to Pull Requests](./docs/pull-requests.md)
- [JavaScript Guidelines](./docs/javascript.md)
- [Secure Development Lifecycle Policy](./docs/sdlc.md)
- [Secure Coding Guidelines](./docs/docs/secure-coding-guidelines.md)
- [TypeScript Guidelines](./docs/typescript.md)
- [Unit Testing Guidelines](./docs/unit-testing.md)
172 changes: 172 additions & 0 deletions docs/secure-coding-guidelines.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
# Secure Coding Guidelines

## Purpose

The secure coding guidelines are a reference to develop, deliver and maintain software in a secure manner.

## Scope

These guidelines apply to developers building client based applications at MetaMask.

## References

The guidelines in this policy were gathered primarily from the [OWASP Top 10](https://owasp.org/www-project-top-ten/) and the [OWASP Application Security Verification Standard](https://owasp.org/www-project-application-security-verification-standard/). We have included just the guidelines most relevant to MetaMask client applications.

## Guidelines

### Input from Users and Third Party Services

#### Data Validation and Sanitization

- Validate and sanitize all user input, and all data from external sources

For example:

- Check that the type matches expectations
- If a value should be a string, do not allow it to be an object
- Validation libraries can be used for more complex types of data
- Check that the value is is an allowed value or within the expected range
- If only a fixed set of values are expected, ensure the value matches one of them
- If a non-negative number is expected, do not allow the value to be negative
- Check that the format matches expectations
- If we expect a 0x-prefixed hexadecimal string, ensure that the 0x is present

- Encode data before output

For example:

- HTML-encode strings before they are included in the DOM
- Some libraries do this automatically (e.g. React)
- URL-encode data before including it in a URL

- When accepting a URI as input, ensure the scheme matches expectations

For example, a URL for a website or API would typically have a scheme of `https`

- Avoid dynamic code execution with untrusted data
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved

Dynamic code execution of untrusted data can allow for injection attacks. If possible, avoid dynamic code execution completely. If you require dynamic code execution, consult with the security team on how to do this safely.

Examples of dynamic code execution:

- `eval`
- `new Function`
- Template processors (e.g. for HTML, SQL, etc.)
- DOM
- `javascript:` protocol

#### Content Security Policy

- Use a Content Security Policy (CSP) to mitigate XSS attacks when rendering external data

#### Data Serialization

- Use pure data formats (e.g. JSON, CSV) for serialization to avoid deserialization attacks

### Data Persistence & Encryption

#### Data Classification

- Classify the data in your application into at least two groups:
- **Non-sensitive** data (e.g. publicly available, transactions, app theme settings, etc.)
- **Sensitive** data (e.g. passwords, keys)

#### Data Security

- Encrypt **sensitive** data prior to persisting
- Limit access to **sensitive** data

#### Encryption

- **Sensitive** data should be secured at rest and in-transit using standard encryption protocols (e.g. cryptography libraries, OS level key management systems, https, etc.)
- Cryptography of **sensitive** data should meet a minimum standard established by the security team

#### Passwords

- Passwords used to decrypt content should adhere to security team recommendations

### Logging & Error Handling

#### Logging

- Secrets and **sensitive** data should not be logged

#### Error Handling

- Avoid including data directly in error messages, especially **sensitive** data

For example, the error "Invalid hex data: \_\_\_\_" might indadvertantly leak a private key. Instead the error could be made more generic ("Invalid hex data"), or you could describe the problem without embedding the data ("Invalid hex data; missing 0x prefix").

### Third Party Integrations

#### Authentication and Authorization

- Do not bundle API tokens in client side applications unless you intend for them to be publicly accessible

#### Integration/Application Integrity

- Monitor integrations/applications for changes and security issues

### Third Party Applications

#### Permissions

- All applications/dapps/snaps shall adhere to the following to Principle of Least Privilege with a default state of allowing no permissions
- Users shall provide consent for required permissions of applications/dapps/snaps

### Dependency Management

#### Dependency Integrity

- Use a lockfile or pinned dependencies to maintain control over which version of each dependency is used

#### Avoid Deprecated and Unmaintained Packages

- Using Socket.dev to check the health and maintenance status of dependencies and seek alternatives when necessary

#### Reduce Dependencies and Keep Them Minimal

- Minimize dependencies to reduce potential vulnerabilities

#### Audit and Monitor Dependencies

- Monitor dependencies for security vulnerabilities and other problems
- Periodically scan for security vulnerabilities (e.g. using tools like `npm audit`)
- Update dependencies quickly when they have security vulnerabilities
- Use Socket.dev to monitor dependencies for other noteworthy changes, such as maintainer changes, or the addition of install scripts or binary files
- Use the following etiquette when addressing Socket.dev warnings:
- Investigate and address all warnings before merging a PR
- Avoid using the `ignore-all` bot command, instead ignoring each warning one at a time
- If you've investigated a warning and found that it's not indicative of a malicious dependency, ignore it with a bot comment and explain your investigation with a short comment
- Contact the security team if you're unsure how to investigate something, or if you'd like to disable a warning category

#### LavaMoat (JavaScript projects only)

- LavaMoat `allow-scripts` should be enabled on all projects
- This project relies upon install scripts being disabled in your package manager. This can be verified by adding the dependency `@lavamoat/preinstall-always-fail`, which will cause installation to fail if scripts are enabled.
Copy link

Choose a reason for hiding this comment

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

allow-scripts setup adds that dependency. Also, yarn 3 and 4 will not fail the entire install process because they swallow errors.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, I hadn't realized that about Yarn v3/v4. I guess your point here is that it'd be safer to advise using allow-scripts setup because users might fail to properly disable install scripts in the package manager otherwise, is that correct? Just wanted to confirm that setting ignore-scripts is also something handled by the setup script.

- `allow-scripts` acts as an allowlist for install scripts. Use the `allow-scripts` binary after installing dependencies to run install scripts on the allowlist.
- On Yarn v3 projects, use the `yarn-plugin-allow-scripts` plugin to run allowed scripts automatically during install
- On other projects, use an npm script called `setup` that will call `install` and `allow-scripts` in sequence
- If you're unsure whether an install script is needed, leave it disabled
- LavaMoat runtime should be used for all Node.js build systems, and LavaMoat bundling tools should be used for all JavaScript client applications
- The LavaMoat runtime helps protect against malicious code in dependencies by isolating dependencies and reducing their capabilities
- The LavaMoat bundling tools will bundle client applications with a LavaMoat runtime and policy
- Regularly review your LavaMoat policies for suspicious permissions
- When the policy is updated, carefully review the diff to ensure that the capabilities granted to each dependency seem appropriate and legitimate
- Consult with the LavaMoat team if you have any questions

### Operations & Infrastructure

#### Project secrets

- Store and manage project secrets securely
- Each CI system should have a method of securely managing secrets
- For secrets used directly by developers, use 1Password
- Limit access to project secrets, following the Principle of Least Authority
- Prevent contributors from committing secrets to the repository
- GitHub has a "Secret scanning" setting that can help with this, and there are third-party tools like `2ms` (too many secrets) that can help as well

#### Application Integrity

- Application should be distributed through approved store portals (e.g. GitHub, App Store, Play Store, Firefox Store, Chrome Store)
- Each distribution channel shall have provide release verifiability (e.g. certificate signing approval, hash verification)