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

[cryptofuzz] Add OpenSSL 1.0.2 and 1.1.0 targets #2454

Merged

Conversation

guidovranken
Copy link
Contributor

There are a bunch of bugs in 1.0.2 and 1.10 that will cause crashes, like openssl/openssl#8980 and openssl/openssl#8972

I still have to create bug reports for the other ones.

You can either merge it now and let ClusterFuzz detect these bugs, or wait until they are all fixed, whichever you prefer.

CC @mattcaswell @kroeckx

I will remove these targets when they reach end-of-life later this year.

@jonathanmetzman
Copy link
Contributor

jonathanmetzman commented May 22, 2019

I'll merge this once the travis failures are fixed.

@guidovranken
Copy link
Contributor Author

Part of the EverCrypt build will not succeed, this is expected, hence we compile with make -j$(nproc) || true:

make -j$(nproc) || true

Apparently Travis trips over this.. @s-zanella can the EverCrypt build script be changed to succeed entirely with sanitizers enabled?

@s-zanella
Copy link

@guidovranken I pushed a change to the PR I've opened in your fork that addresses this.

You can save a lot of time (and space in logs) by only compiling libcrypto.a in OpenSSL builds. Unfortunately, there is no recipe that works across all versions, but I believe make build_generated libcrypto.a works for OpenSSL 1.1.x and make build_crypto works for OpenSSL 1.0.2 (see openssl/openssl#4597).

Copy link

@s-zanella s-zanella left a comment

Choose a reason for hiding this comment

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

This saves time by building only OpenSSL libcrypto.a

cd $SRC/openssl-OpenSSL_1_1_0-stable/
./config --debug enable-md2 enable-rc5 $CFLAGS
make depend
make -j$(nproc)

Choose a reason for hiding this comment

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

Suggested change
make -j$(nproc)
make -j$(nproc) build_generated libcrypto.a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I apply this I get errors like

include/openssl/bio.h:667:27: warning: declaration of 'struct hostent' will not be visible outside of this function [-Wvisibility]
DEPRECATEDIN_1_1_0(struct hostent *BIO_gethostbyname(const char *name))
                          ^
include/openssl/bio.h:668:1: error: expected function body after function declarator
DEPRECATEDIN_1_1_0(int BIO_get_port(const char *str, unsigned short *port_ptr))
^
include/openssl/bio.h:680:55: warning: declaration of 'union BIO_sock_info_u' will not be visible outside of this function [-Wvisibility]
                  enum BIO_sock_info_type type, union BIO_sock_info_u *info);
                                                      ^
In file included from crypto/aes/aes_wrap.c:10:
In file included from crypto/include/internal/cryptlib.h:26:
include/openssl/bio.h:667:27: warning: declaration of 'struct hostent' will not be visible outside of this function [-Wvisibility]
DEPRECATEDIN_1_1_0(struct hostent *BIO_gethostbyname(const char *name))
                          ^
include/openssl/bio.h:668:1: error: expected function body after function declarator
DEPRECATEDIN_1_1_0(int BIO_get_port(const char *str, unsigned short *port_ptr))
^
include/openssl/bio.h:680:55: warning: declaration of 'union BIO_sock_info_u' will not be visible outside of this function [-Wvisibility]
                  enum BIO_sock_info_type type, union BIO_sock_info_u *info);
                                                      ^
In file included from crypto/aes/aes_ige.c:10:
In file included from crypto/include/internal/cryptlib.h:27:
include/openssl/err.h:249:1: error: expected function body after function declarator
DEPRECATEDIN_1_0_0(void ERR_remove_state(unsigned long pid))
^
In file included from crypto/aes/aes_wrap.c:10:
In file included from crypto/include/internal/cryptlib.h:27:
include/openssl/err.h:249:1: error: expected function body after function declarator
DEPRECATEDIN_1_0_0(void ERR_remove_state(unsigned long pid))
^
crypto/aes/aes_ige.c:47:5: error: use of undeclared identifier 'OPENSSL_FILE'; did you mean 'OPENSSL_die'?
    OPENSSL_assert(in && out && key && ivec);
    ^
include/openssl/crypto.h:332:60: note: expanded from macro 'OPENSSL_assert'
    (void)((e) ? 0 : (OPENSSL_die("assertion failed: " #e, OPENSSL_FILE, OPENSSL_LINE), 1))

make clean || true
./config --debug no-asm enable-md2 enable-rc5 $CFLAGS
make depend
make -j$(nproc)

Choose a reason for hiding this comment

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

Suggested change
make -j$(nproc)
make -j$(nproc) build_generated libcrypto.a

cd $SRC/openssl-OpenSSL_1_0_2-stable/
./config --debug enable-md2 enable-rc5 $CFLAGS
make depend
make -j$(nproc)

Choose a reason for hiding this comment

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

Suggested change
make -j$(nproc)
make -j$(nproc) build_crypto

make clean || true
./config --debug no-asm enable-md2 enable-rc5 $CFLAGS
make depend
make -j$(nproc)
Copy link

@s-zanella s-zanella May 23, 2019

Choose a reason for hiding this comment

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

Suggested change
make -j$(nproc)
make -j$(nproc) build_crypto

@guidovranken
Copy link
Contributor Author

Thanks for the fixes @s-zanella . I tried your build speedups but I got errors so I'm leaving them out for now, will try again later.

@jonathanmetzman Travis now fails because "The job exceeded the maximum log length, and has been terminated.". Can't do anything about that now unless you want me to redirect output to /dev/null.

cd $SRC/evercrypt/dist/generic
make -j$(nproc) || true
cd $SRC/evercrypt/dist
make -C portable -j$(npro) libevercrypt.a

Choose a reason for hiding this comment

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

npro -> nproc

@s-zanella
Copy link

Thanks for the fixes @s-zanella . I tried your build speedups but I got errors so I'm leaving them out for now, will try again later.

Strange. What errors do you get?
I built and checked the builds of all fuzzers using the infra/helper.py script with these changes and didn't get any errors:
s-zanella@e257632

Overall build logs are much shorter. For instance, the log for --engine libfuzzer --sanitizer memory goes from 22.9k down to 14.3k lines. You can also silence the output of unzip with unzip -q to save some more space (~5k lines).

@jonathanmetzman
Copy link
Contributor

@jonathanmetzman Travis now fails because "The job exceeded the maximum log length, and has been terminated.". Can't do anything about that now unless you want me to redirect output to /dev/null.

I'm not sure how to solve this in a general way. Maybe we should add -Wno-warnings to CFLAGS and CXXFLAGS on travis? You can try this yourself if you'd like, or adding -q to the unzip and apt-get commands (as well as removing the v argument to tar) should save us some unnecessary output.

If you don't want to go this trouble I can merge and see if it fails to build the old-fashioned way.

@jonathanmetzman
Copy link
Contributor

btw, I checked that silencing these particular commands cause appropriate error messages to be printed on error. But if you feel like the verbose output provides value, feel to not silence them and I will merge this patch even though it fails travis (some projects will take too long on travis, so I don't expect every patch to pass on travis).

@guidovranken
Copy link
Contributor Author

@jonathanmetzman all Travis builds pass now.

Copy link
Contributor

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathanmetzman jonathanmetzman merged commit e5280ac into google:master May 31, 2019
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.

3 participants