Skip to content

Fix critical stack overflow arising from VLA usage#1242

Merged
hugbubby merged 1 commit into
TokTok:masterfrom
hugbubby:zoff-overflow-patch
Dec 12, 2018
Merged

Fix critical stack overflow arising from VLA usage#1242
hugbubby merged 1 commit into
TokTok:masterfrom
hugbubby:zoff-overflow-patch

Conversation

@hugbubby

@hugbubby hugbubby commented Nov 4, 2018

Copy link
Copy Markdown
Member

Two instances of VLA usage in the encrypt/decrypt functions of crypto_core left users vulnerable to a stack overflow. Here is a patch and addition to the encryptsave test to make sure it doesn't reappear.


This change is Reviewable

@hugbubby hugbubby changed the title Fix critical stack overflow arising from VLA usage WIP: Fix critical stack overflow arising from VLA usage Nov 4, 2018
@hugbubby hugbubby force-pushed the zoff-overflow-patch branch from ddb6436 to edb9001 Compare November 4, 2018 17:27
@codecov

codecov Bot commented Nov 4, 2018

Copy link
Copy Markdown

Codecov Report

Merging #1242 into master will increase coverage by 0.4%.
The diff coverage is 82.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1242     +/-   ##
========================================
+ Coverage    83.2%   83.6%   +0.4%     
========================================
  Files          82      82             
  Lines       14827   14867     +40     
========================================
+ Hits        12342   12436     +94     
+ Misses       2485    2431     -54
Impacted Files Coverage Δ
auto_tests/encryptsave_test.c 95.7% <100%> (+0.6%) ⬆️
toxcore/crypto_core.c 91.4% <72.4%> (-8.6%) ⬇️
toxcore/LAN_discovery.c 84.9% <0%> (-1%) ⬇️
toxcore/Messenger.c 86.9% <0%> (+0.1%) ⬆️
toxav/toxav.c 68.4% <0%> (+0.3%) ⬆️
toxcore/onion_client.c 96.1% <0%> (+0.7%) ⬆️
toxav/msi.c 65.3% <0%> (+0.8%) ⬆️
toxcore/net_crypto.c 95% <0%> (+1.2%) ⬆️
toxcore/TCP_server.c 82.9% <0%> (+2.5%) ⬆️
toxcore/TCP_client.c 67.5% <0%> (+2.8%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f8f82a...b4144fa. Read the comment docs.

@hugbubby hugbubby force-pushed the zoff-overflow-patch branch 4 times, most recently from 5d91f6f to a2f5688 Compare November 4, 2018 20:05
@iphydf iphydf added this to the v0.2.x milestone Nov 4, 2018
@hugbubby hugbubby force-pushed the zoff-overflow-patch branch 4 times, most recently from 057f0ef to c189be1 Compare November 4, 2018 20:52
@hugbubby hugbubby changed the title WIP: Fix critical stack overflow arising from VLA usage Fix critical stack overflow arising from VLA usage Nov 4, 2018
@hugbubby hugbubby force-pushed the zoff-overflow-patch branch 2 times, most recently from a881614 to ed044cf Compare November 9, 2018 19:48
@hugbubby hugbubby requested a review from zugz November 10, 2018 15:59
@zugz

zugz commented Nov 10, 2018 via email

Copy link
Copy Markdown

@hugbubby hugbubby force-pushed the zoff-overflow-patch branch from ed044cf to 242b53d Compare November 10, 2018 17:04
@hugbubby

Copy link
Copy Markdown
Member Author

@zugz crypto_memzeroed it for now.

@iphydf iphydf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @hugbubby and @zugz)


auto_tests/encryptsave_test.c, line 158 at r2 (raw file):

    int ciphertext_length2a = size_large + TOX_PASS_ENCRYPTION_EXTRA_LENGTH;
    int plaintext_length2a = size_large;
    uint8_t *encrypted2a = (uint8_t *) malloc(ciphertext_length2a);

No space after the cast.


auto_tests/encryptsave_test.c, line 164 at r2 (raw file):

    free(in_plaintext2a);

    //Decryption of same message.

Space after //.


toxcore/crypto_core.c, line 159 at r2 (raw file):

    }

    size_t size_temp_plain = length + crypto_box_ZEROBYTES;

const


toxcore/crypto_core.c, line 162 at r2 (raw file):

    size_t size_temp_encrypted = length + crypto_box_MACBYTES + crypto_box_BOXZEROBYTES;

    uint8_t *temp_plain = crypto_malloc(size_temp_plain);

uint8_t *const temp...

@hugbubby hugbubby force-pushed the zoff-overflow-patch branch 2 times, most recently from 66cca94 to 05ea03e Compare November 18, 2018 18:19
@hugbubby

hugbubby commented Nov 18, 2018

Copy link
Copy Markdown
Member Author

@iphydf I... (think?) I implemented your requests correctly? Travis is complaining about a build but it seems to be unrelated.

Thank you for doing the crypto_malloc and crypto_memzero, by the way. I know I said I would but somehow was under the impression that you wanted me to do it after this got merged. Sorry about that.

@zoff99 zoff99 mentioned this pull request Dec 4, 2018
@zoff99

zoff99 commented Dec 4, 2018

Copy link
Copy Markdown

@iphydf why is this critical fix not merged yet? can you have a look please.

@iphydf iphydf force-pushed the zoff-overflow-patch branch 3 times, most recently from 978df33 to 8ad3481 Compare December 4, 2018 23:09

@tvladyslav tvladyslav left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One small check just in case.

Comment thread auto_tests/encryptsave_test.c Outdated
@hugbubby hugbubby force-pushed the zoff-overflow-patch branch from 8ad3481 to 9d6b351 Compare December 11, 2018 03:21
@hugbubby

Copy link
Copy Markdown
Member Author

@iphydf @zoff99 I can't merge this until we fix travis's clang build.

@iphydf iphydf changed the title Fix critical stack overflow arising from VLA usage WIP: Fix critical stack overflow arising from VLA usage Dec 11, 2018
@iphydf iphydf changed the title WIP: Fix critical stack overflow arising from VLA usage Fix critical stack overflow arising from VLA usage Dec 11, 2018
@iphydf

iphydf commented Dec 11, 2018

Copy link
Copy Markdown
Member

@hugbubby it's green now, but the commits aren't signed.

@hugbubby hugbubby force-pushed the zoff-overflow-patch branch 5 times, most recently from 77904ca to c0514a9 Compare December 12, 2018 20:52
Also added and used the new crypto_malloc and crypto_free.

The latter also zeroes out the memory safely. The former only exists for
symmetry (static analysis can detect asymmetric usages).
@hugbubby hugbubby force-pushed the zoff-overflow-patch branch from c0514a9 to b4144fa Compare December 12, 2018 21:10
@hugbubby hugbubby merged commit b4144fa into TokTok:master Dec 12, 2018
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.9 Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants