Skip to content

Stop transactions from being reentrant #316

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

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

Patater
Copy link
Contributor

@Patater Patater commented Nov 7, 2019

We want to explicitly disallow creating new transactions when a
transaction is already in progress. However, we were incorrectly
checking for the existence of the injected entropy file before
continuing with creating a transaction. This meant we could have a
transaction already in progress and would be able to still create a new
transaction. It also meant we couldn't start a new transaction if any
entropy had been injected. Check the transaction file instead of the
injected entropy file in order to prevent multiple concurrent
transactions.

We want to explicitly disallow creating new transactions when a
transaction is already in progress. However, we were incorrectly
checking for the existence of the injected entropy file before
continuing with creating a transaction. This meant we could have a
transaction already in progress and would be able to still create a new
transaction. It also meant we couldn't start a new transaction if any
entropy had been injected. Check the transaction file instead of the
injected entropy file in order to prevent multiple concurrent
transactions.
@Patater Patater added the bug Something isn't working label Nov 7, 2019
Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Ooops. (This is why we need tests!)

@gilles-peskine-arm gilles-peskine-arm added the needs: review The pull request is ready for review. This generally means that it has no known issues. label Nov 7, 2019
@gilles-peskine-arm gilles-peskine-arm added needs: ci Needs a passing full CI run and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Nov 8, 2019
@Patater Patater removed the needs: ci Needs a passing full CI run label Nov 8, 2019
@Patater
Copy link
Contributor Author

Patater commented Nov 8, 2019

Merge job is green. Mergin'.

@Patater Patater merged commit b14a4ff into ARMmbed:development Nov 8, 2019
gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Nov 15, 2019
* ARMmbed#292: Make psa_close_key(0) and psa_destroy_key(0) succeed
* ARMmbed#299: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed
* ARMmbed#259: Check `len` against buffers size upper bound in PSA tests
* ARMmbed#288: Add ECDSA tests with hash and key of different lengths
* ARMmbed#305: CTR_DRBG: grab a nonce from the entropy source if needed
* ARMmbed#316: Stop transactions from being reentrant
* ARMmbed#317: getting_started: Make it clear that keys are passed in
* ARMmbed#314: Fix pk_write with EC key to use a constant size for the private value
* ARMmbed#298: Test a build without any asymmetric cryptography
* ARMmbed#284: Fix some possibly-undefined variable warnings
* ARMmbed#315: Define MBEDTLS_PK_SIGNATURE_MAX_SIZE
* ARMmbed#318: Finish side-porting commits from mbedtls-restricted that missed the split
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants