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

Fix memcpy on overlapping addresses in BuildUserAuthRequestEcc #407

Closed
wants to merge 1 commit into from

Conversation

landhb
Copy link

@landhb landhb commented Apr 24, 2022

Good afternoon,

I've been attempting to use WolfSSH with ECC keys and was following the example and ran into this issue. BuildUserAuthRequestEcc seems to be causing a call to memcpy with two overlapping addresses in DecodeECC_DSA_Sig_Bin. AddressSanitizer reports the issue in the current client example as well.

Reproduction steps:

Build with address sanitizer

CPPFLAGS="-g -fsanitize=address -fno-omit-frame-pointer" LDFLAGS="-fsanitize=address" LIBS="-lpthread" ./configure --with-wolfssl=./install/usr/local --disable-shared --enable-debug

Run example client with ECC

examples/client/client -h 127.0.0.1 -p 22222 -u hansel -e

Address sanitizer output:

=================================================================
==25876==ERROR: AddressSanitizer: memcpy-param-overlap: memory ranges [0x7ffd81558220,0x7ffd81558240) and [0x7ffd81558224, 0x7ffd81558244) overlap
    #0 0x7f1155052105  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x3f105)
    #1 0x7f1154f3e567 in DecodeECC_DSA_Sig_Bin (install/usr/local/lib/libwolfssl.so.32+0x3f567)
    #2 0x7f1154f59cca in wc_ecc_sig_to_rs (install/usr/local/lib/libwolfssl.so.32+0x5acca)
    #3 0x7f1154fe26de in BuildUserAuthRequestEcc src/internal.c:8603
    #4 0x7f1154fe26de in BuildUserAuthRequestPublicKey src/internal.c:8741
    #5 0x7f1154fe26de in SendUserAuthRequest src/internal.c:8906
    #6 0x7f1154ff26f4 in DoUserAuthFailure src/internal.c:4684
    #7 0x7f1154ff26f4 in DoPacket src/internal.c:5656
    #8 0x7f1154ff4c44 in DoReceive src/internal.c:6252
    #9 0x7f1154fc3887 in wolfSSH_connect src/ssh.c:788
    #10 0x56098955e5c4 in client_test examples/client/client.c:1075
    #11 0x56098955c708 in main examples/client/client.c:1196
    #12 0x7f1154d3d09a in __libc_start_main ../csu/libc-start.c:308
    #13 0x56098955c849 in _start (/mnt/archive/Documents/Projects/wolfssh/examples/client/.libs/client+0x4849)

Address 0x7ffd81558220 is located in stack of thread T0 at offset 704 in frame
    #0 0x7f1154fdfdef in SendUserAuthRequest src/internal.c:8778

  This frame has 12 object(s):
    [32, 36) 'idx'
    [96, 100) 'idx'
    [160, 164) 'sigSz'
    [224, 228) 'rSz'
    [288, 292) 'sSz'
    [352, 416) 'digest'
    [448, 512) 'digest'
    [544, 672) 'authData'
    [704, 843) 'sig' <== Memory access at offset 704 is inside this variable
    [896, 1312) 'hash'
    [1344, 1760) 'hash'
    [1792, 2304) 'encDigest'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
Address 0x7ffd81558224 is located in stack of thread T0 at offset 708 in frame
    #0 0x7f1154fdfdef in SendUserAuthRequest src/internal.c:8778

  This frame has 12 object(s):
    [32, 36) 'idx'
    [96, 100) 'idx'
    [160, 164) 'sigSz'
    [224, 228) 'rSz'
    [288, 292) 'sSz'
    [352, 416) 'digest'
    [448, 512) 'digest'
    [544, 672) 'authData'
    [704, 843) 'sig' <== Memory access at offset 708 is inside this variable
    [896, 1312) 'hash'
    [1344, 1760) 'hash'
    [1792, 2304) 'encDigest'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: memcpy-param-overlap (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x3f105)
==25876==ABORTING

@wolfSSL-Bot
Copy link

Can one of the wolfSSL admins verify this patch?

@embhorn
Copy link
Member

embhorn commented Apr 25, 2022

Hi @landhb

Thanks for this excellent report! I have requested a review by our team. We do not typically accept small code contributions, but after review, we can incorporate this fix into an existing PR.

@landhb
Copy link
Author

landhb commented Apr 25, 2022

Sounds good! Thanks!

@ejohnstown
Copy link
Contributor

This is going to be fixed with PR #409. There's a few other things we need to deal with when adding the new buffer w.r.t. the small stack build option. Thank you for the bug report!

@ejohnstown ejohnstown closed this May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants