Skip to content

Conversation

@tiandrey
Copy link

Added new extensions to enum tlsext_index_en in ssl/ssl_locl.h according to comments in source code - now tests in debian package pass without errors.

Added new extensions to enum tlsext_index_en in ssl/ssl_locl.h - now tests in debian package pass without errors
@fooinha
Copy link
Owner

fooinha commented Nov 11, 2020

I'll take a look.

@fooinha fooinha merged commit 5c973b0 into fooinha:master Nov 11, 2020
@kedare
Copy link

kedare commented Nov 19, 2020

This broke the patching for me

patching file include/openssl/tls1.h
patching file ssl/statem/extensions.c
can't find file to patch at input line 45
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff -upr openssl-1.1.1d_orig/ssl/ssl_locl.h openssl-1.1.1d/ssl/ssl_locl.h
|--- openssl-1.1.1d_orig/ssl/ssl_locl.h  2020-10-26 18:19:43.157168940 +0300
|+++ openssl-1.1.1d/ssl/ssl_locl.h       2020-11-10 18:49:14.150574957 +0300
--------------------------

It was working fine, is there any other change required for this to work ?

@tiandrey
Copy link
Author

@kedare , what version of openssl are you using?

@kedare
Copy link

kedare commented Nov 19, 2020

OpenSSL 1.1.1

@tiandrey
Copy link
Author

Please be more specific. My patch is for 1.1.1d, which is also 1.1.1.
Version is specified in include/openssl/opensslv.h:

openssl-1.1.1d_orig $ grep OPENSSL_VERSION include/openssl/opensslv.h
# define OPENSSL_VERSION_NUMBER  0x1010104fL
# define OPENSSL_VERSION_TEXT    "OpenSSL 1.1.1d  10 Sep 2019"

@fooinha
Copy link
Owner

fooinha commented Nov 19, 2020

yes, @kedare , can you please give more details, system wide or for the full build process.

This passed the travis tests and I've also tested manually.

Thanx.

@kedare
Copy link

kedare commented Nov 19, 2020

I am using it on top of the openresty/opensresty:1.17.8.2-5-buster image version, and building over the OpenSSL_1_1_1-stable branch of the https://github.com/openssl/openssl repository.
And for the openresty part, I am building the 1.17.8.2 source code (directly from the source .tar.gz)

This is the failing step :

RUN git clone --depth 1 https://github.com/SpiderLabs/ModSecurity-nginx.git && \
    curl -O https://openresty.org/download/openresty-${OPENRESTY_VERSION}.tar.gz && \
    tar -xf openresty-${OPENRESTY_VERSION}.tar.gz && \
    rm openresty-${OPENRESTY_VERSION}.tar.gz && \
    git clone https://github.com/openssl/openssl.git && \
    cd /openssl && \
    git checkout OpenSSL_${OPENSSL_VERSION}-stable && \
    cd / && \
    git clone https://github.com/fooinha/nginx-ssl-ja3.git && \
    cd /nginx-ssl-ja3 && \
    git checkout ${NGINX_JA3_COMMIT} && \
    cd /openssl && \
    cp /nginx-ssl-ja3/patches/openssl.extensions.patch . && \
    patch -p1 < openssl.extensions.patch && \
    cd /openresty-${OPENRESTY_VERSION}/bundle/nginx-1*/ && \
    cp /nginx-ssl-ja3/patches/nginx.latest.patch . && \
    patch -p1 < nginx.latest.patch && \
    cd /openresty-${OPENRESTY_VERSION}
OPENRESTY_VERSION=1.17.8.2
OPENSSL_VERSION=1_1_1

(I added the current checkout as workaround the nginx-ssl-ja3)

@tiandrey
Copy link
Author

tiandrey commented Nov 19, 2020

Seems like OpenSSL developers have changed name of that file from ssl_locl.h in 1.1.1d to ssl_local.h in 1.1.1e.
I'll create new patch for new versions of openssl, for now you can fix the patch yourself. I do not recommend using old version of this patch because number of extensions matters, and it's possible that old patch introduces memory leaking or out-of-bounds memory access, or at least it breaks some functionality (otherwise the tests wouldn't have failed).

UPD: specific commit that renames files and breaks the patch: openssl/openssl@706457b

@tiandrey
Copy link
Author

#23

@kedare
Copy link

kedare commented Nov 19, 2020

Thanks, indeed it's a temporary workaround, I will use your version once merged :)

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