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

Mark MASTG-TEST-0016 as covered by v2 (by @guardsquare) #3026

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
2 changes: 2 additions & 0 deletions Document/0x05e-Testing-Cryptography.md
Original file line number Diff line number Diff line change
Expand Up @@ -242,3 +242,5 @@ Cryptography requires secure pseudo random number generation (PRNG). Standard Ja
In general, `SecureRandom` should be used. However, if the Android versions below Android 4.4 (API level 19) are supported, additional care needs to be taken in order to work around the bug in Android 4.1-4.3 (API level 16-18) versions that [failed to properly initialize the PRNG](https://android-developers.googleblog.com/2013/08/some-securerandom-thoughts.html "Some SecureRandom Thoughts").

Most developers should instantiate `SecureRandom` via the default constructor without any arguments. Other constructors are for more advanced uses and, if used incorrectly, can lead to decreased randomness and security. The PRNG provider backing `SecureRandom` uses the `SHA1PRNG` from `AndroidOpenSSL` (Conscrypt) provider.

Check the [Android Documentation](https://developer.android.com/privacy-and-security/risks/weak-prng) for more details.
5 changes: 4 additions & 1 deletion mitigations/android-use-secure-random.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,7 @@ platform: android

[`java.security.SecureRandom`](https://developer.android.com/reference/java/security/SecureRandom) uses SHA1PRNG by default to produce non-deterministic results from a seed based on system thread timing obtained from `dev/urandom`. This seeding occurs automatically during object construction or acquisition, eliminating the need for explicit seeding of the PRNG.

The default constructor is usually sufficient for generating secure random values. However, while other constructors are available for advanced use cases, their improper use could reduce the randomness of the output. Therefore, non-default constructors should be used with caution.
The default [no-argument constructor of `SecureRandom`](https://wiki.sei.cmu.edu/confluence/display/java/MSC02-J.+Generate+strong+random+numbers "Generation of Strong Random Numbers") is usually recommended for generating secure random values, as it uses the system-specified seed value to generate a 128-byte-long random number.

Providing an hard coded seed to the constructor of `SecureRandom` is [discouraged in Android Documentation](https://developer.android.com/privacy-and-security/risks/weak-prng?source=studio#weak-prng-java-security-securerandom), as it may lead to introducing deterministic behavior of `SecureRandom` in some implementations.
nmsa marked this conversation as resolved.
Show resolved Hide resolved
`SecureRandom` documentation explains that [the provided seed supplements, rather than replacing the existing seed](https://developer.android.com/reference/java/security/SecureRandom?hl=en#setSeed(byte[])), but this may not apply if an [old security provider](https://android-developers.googleblog.com/2016/06/security-crypto-provider-deprecated-in.html) is being used.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I included this to cover for this weird corner case, so that the mitigation explains all the relevant points.

nmsa marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 8 additions & 4 deletions tests-beta/android/MASVS-CRYPTO/MASTG-TEST-0204.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
platform: android
title: Insecure Random API Usage
id: MASTG-TEST-0204
type: [static]
type: [static, dynamic]
mitigations:
- android-use-secure-random
prerequisites:
Expand All @@ -13,16 +13,20 @@ weakness: MASWE-0027

## Overview

Android apps sometimes use insecure pseudorandom number generators (PRNGs) such as `java.util.Random`, which is essentially a linear congruential generator. This type of PRNG generates a predictable sequence of numbers for any given seed value, making the sequence reproducible and insecure for cryptographic use. In particular, `java.util.Random` and `Math.random()` ([the latter](https://franklinta.com/2014/08/31/predicting-the-next-math-random-in-java/) simply calling `nextDouble()` on a static `java.util.Random` instance) produce identical number sequences when initialized with the same seed across all Java implementations.
Android apps sometimes use insecure [pseudorandom number generators (PRNGs)](../../../Document/0x05e-Testing-Cryptography.md#random-number-generation) such as [`java.util.Random`](https://developer.android.com/reference/java/util/Random), which is essentially a linear congruential generator. This type of PRNG generates a predictable sequence of numbers for any given seed value, making the sequence reproducible and insecure for cryptographic use. In particular, `java.util.Random` and `Math.random()` ([the latter](https://franklinta.com/2014/08/31/predicting-the-next-math-random-in-java/) simply calling `nextDouble()` on a static `java.util.Random` instance) produce identical number sequences when initialized with the same seed across all Java implementations.
nmsa marked this conversation as resolved.
Show resolved Hide resolved
In general, if a PRNG is not advertised as being cryptographically secure, then it is probably a statistical PRNG and should not be used in security-sensitive contexts.
See the [Android Documentation](https://developer.android.com/privacy-and-security/risks/weak-prng) and the guide on ["random number generation"](../../../Document/0x05e-Testing-Cryptography.md#random-number-generation) for details.

## Steps

1. Run a static analysis (@MASTG-TECH-0014) tool on the app and look for insecure random APIs.
1. Run a static analysis (@MASTG-TECH-0014) tool on the app and look for insecure random APIs, or you can use @MASTG-TECH-0033 to detect the use of such APIs.
2. For each of the identified API uses, check if they are used in a security relevant context.
For this, you can reverse engineer the app (@MASTG-TECH-0017) and inspect the code(@MASTG-TECH-0023).

## Observation

The output should contain a list of locations where insecure random APIs are used.

## Evaluation

The test case fails if you can find random numbers generated using those APIs that are used in security-relevant contexts.
The test case fails if you can find random numbers generated using those APIs that are used in security-relevant contexts, such as generating passwords or authentication tokens.
3 changes: 3 additions & 0 deletions tests/android/MASVS-CRYPTO/MASTG-TEST-0016.md
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please double check:

  1. Do we have a new test that tests for "Identify all instances of SecureRandom that are not created using the default constructor. Specifying the seed value may reduce randomness." (paragraph 2). If not, we should add one.
  2. Are we using all the links from this test in the new ones?
  1. Do we have some indication about "You can use @MASTG-TECH-0033 on the mentioned classes and methods to determine input / output values being used.". This may be useful to determine if it's a security-relevant context. Otherwise, we should reverse engineer the code surrounding the found methods to determine if they seem to be used in a security-relevant context. (adding this could be as simple as adding a short paragraph in the Evaluation section of MASTG-TEST-0204)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. The suggestion does not apply anymore. The recent documentation about SecureRandom explains that the provided seed supplements the seed, does not replace it. I was not sure where to document this.
  2. Will add the missing ones.
  3. Will adjust

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After checking, both CMU links (which I did not include) are currently very outdated, and their recommendations do not apply anymore. I am mentioning one of them in the mitigations, but tbh even there some of the recommendations are slightly incorrect.

Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ title: Testing Random Number Generation
masvs_v1_levels:
- L1
- L2
status: deprecated
covered_by: ['MASTG-TEST-0204', 'MASTG-TEST-0205']
deprecation_reason: New version available in MASTG V2
---

## Overview
Expand Down