openvpn-devel Mailing List for OpenVPN

Attention is currently required from: plaisthos. Hello plaisthos, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/789?usp=email to review the following change. Change subject: GHA: General update November 2024 ...................................................................... GHA: General update November 2024 Contains the following renovate updates: - Update dependency libressl/portable to v4 - Update dependency Mbed-TLS/mbedtls to v3.6.2 - Update mingw ubuntu runner to v24 - Do NOT update the uncrustify runner since newer uncrustify is not usable with the current config - Update vcpkg digest to b505fa7 Additionally change the action reference pinning to consistently refer to the tags instead of the branches. Change-Id: I91f68317450c3c0d69be2c489276739211ccb422 Signed-off-by: Frank Lichtenheld <fr...@li...> --- M .github/workflows/build.yaml M .github/workflows/coverity-scan.yml 2 files changed, 24 insertions(+), 24 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/89/789/1 diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 3958ef0..bb13ab3 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -13,7 +13,7 @@ - name: Install dependencies run: sudo apt update && sudo apt install -y uncrustify - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 with: path: openvpn - name: Show uncrustify version @@ -27,7 +27,7 @@ - name: Show changes on standard output run: git diff working-directory: openvpn - - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4 + - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 with: name: uncrustify-changes.patch path: 'openvpn/uncrustify-changes.patch' @@ -42,19 +42,19 @@ arch: [x86, x64] name: "gcc-mingw - ${{ matrix.arch }} - OSSL" - runs-on: ubuntu-22.04 + runs-on: ubuntu-24.04 env: VCPKG_ROOT: ${{ github.workspace }}/vcpkg steps: - name: Install dependencies run: sudo apt update && sudo apt install -y mingw-w64 unzip cmake ninja-build build-essential wget python3-docutils man2html-base - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Restore from cache and install vcpkg uses: lukka/run-vcpkg@5e0cab206a5ea620130caf672fce3e4a6b5666a1 # v11.5 with: - vcpkgGitCommitId: 8d3649ba34aab36914ddd897958599aa0a91b08e + vcpkgGitCommitId: b505fa789fd96eb5496a2e42c651c169e8460d27 vcpkgJsonGlob: '**/mingw/vcpkg.json' - name: Run CMake with vcpkg.json manifest @@ -64,7 +64,7 @@ buildPreset: mingw-${{ matrix.arch }} buildPresetAdditionalArgs: "['--config Debug']" - - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4 + - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 with: name: openvpn-mingw-${{ matrix.arch }} path: | @@ -72,7 +72,7 @@ ${{ github.workspace }}/out/build/mingw/${{ matrix.arch }}/Debug/*.dll !${{ github.workspace }}/out/build/mingw/${{ matrix.arch }}/Debug/test_*.exe - - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4 + - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 with: name: openvpn-mingw-${{ matrix.arch }}-tests path: | @@ -91,9 +91,9 @@ name: "mingw unittest ${{ matrix.test }} - ${{ matrix.arch }} - OSSL" steps: - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Retrieve mingw unittest - uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 + uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8 with: name: openvpn-mingw-${{ matrix.arch }}-tests path: unittests @@ -165,7 +165,7 @@ - name: Install dependencies run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev liblz4-dev libcap-ng-dev libnl-genl-3-dev linux-libc-dev man2html libcmocka-dev python3-docutils libtool automake autoconf ${SSLPKG} ${PKCS11PKG} - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: autoconf run: autoreconf -fvi - name: configure @@ -195,7 +195,7 @@ - name: Install dependencies run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev liblz4-dev libcap-ng-dev libnl-genl-3-dev linux-libc-dev man2html clang libcmocka-dev python3-docutils libtool automake autoconf libmbedtls-dev - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: autoconf run: autoreconf -fvi - name: configure @@ -235,7 +235,7 @@ - name: Install dependencies run: brew install ${{matrix.ssllib}} lzo lz4 man2html cmocka libtool automake autoconf - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Set environment run: | cat >>$GITHUB_ENV <<EOF; @@ -267,7 +267,7 @@ runs-on: windows-latest steps: - - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - uses: lukka/get-cmake@070a0507a7abe157ef918deec391da1be197d2d1 # v3.30.3 - name: Install rst2html @@ -276,7 +276,7 @@ - name: Restore artifacts, or setup vcpkg (do not install any package) uses: lukka/run-vcpkg@5e0cab206a5ea620130caf672fce3e4a6b5666a1 # v11.5 with: - vcpkgGitCommitId: 8d3649ba34aab36914ddd897958599aa0a91b08e + vcpkgGitCommitId: b505fa789fd96eb5496a2e42c651c169e8460d27 vcpkgJsonGlob: '**/windows/vcpkg.json' - name: Run CMake with vcpkg.json manifest (NO TESTS) @@ -295,7 +295,7 @@ testPreset: win-${{ matrix.arch }}-release testPresetAdditionalArgs: "['--output-on-failure']" - - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4 + - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 with: name: openvpn-msvc-${{ matrix.arch }} path: | @@ -335,12 +335,12 @@ - name: Install dependencies run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev liblz4-dev linux-libc-dev man2html clang libcmocka-dev python3-docutils libtool automake autoconf pkg-config libcap-ng-dev libnl-genl-3-dev - name: "libressl: checkout" - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 with: path: libressl # versioning=semver-coerced repository: libressl/portable - ref: v3.8.3 + ref: v4.0.0 - name: "libressl: autogen.sh" run: ./autogen.sh working-directory: libressl @@ -356,7 +356,7 @@ - name: "ldconfig" run: sudo ldconfig - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: autoconf run: autoreconf -fvi - name: configure @@ -397,13 +397,13 @@ - name: Install dependencies run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev liblz4-dev linux-libc-dev man2html clang libcmocka-dev python3-docutils python3-jinja2 python3-jsonschema libtool automake autoconf pkg-config libcap-ng-dev libnl-genl-3-dev - name: "mbedtls: checkout" - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 with: path: mbedtls submodules: true # versioning=semver-coerced repository: Mbed-TLS/mbedtls - ref: v3.6.1 + ref: v3.6.2 - name: "mbedtls: make no_test" run: make -j3 no_test SHARED=1 working-directory: mbedtls @@ -411,7 +411,7 @@ run: sudo make install DESTDIR=/usr working-directory: mbedtls - name: Checkout OpenVPN - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: autoconf run: autoreconf -fvi - name: configure diff --git a/.github/workflows/coverity-scan.yml b/.github/workflows/coverity-scan.yml index 16c979d..999fd29 100644 --- a/.github/workflows/coverity-scan.yml +++ b/.github/workflows/coverity-scan.yml @@ -13,7 +13,7 @@ steps: - name: Check submission cache id: check_submit - uses: actions/cache/restore@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4 + uses: actions/cache/restore@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.0.2 with: path: | cov-int @@ -25,7 +25,7 @@ - name: Checkout OpenVPN if: steps.check_submit.outputs.cache-hit != 'true' - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Download Coverity Build Tool if: steps.check_submit.outputs.cache-hit != 'true' @@ -65,7 +65,7 @@ - name: Cache submission if: steps.check_submit.outputs.cache-hit != 'true' - uses: actions/cache/save@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4 + uses: actions/cache/save@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.0.2 with: path: | cov-int -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/789?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I91f68317450c3c0d69be2c489276739211ccb422 Gerrit-Change-Number: 789 Gerrit-PatchSet: 1 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-MessageType: newchange 

Attention is currently required from: flichtenheld, plaisthos. ordex has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/788?usp=email ) Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... Patch Set 5: (1 comment) Commit Message: http://gerrit.openvpn.net/c/openvpn/+/788/comment/cbc959de_21931c3f : PS3, Line 19: (Above warnings are critical on Fedora 40 as they are turned into errors) > my intent was to say that this is turned into an error on Fedora by default - the reason resides in […] I decided to drop that sentence at all. We all know that -Werror will make warnings critical. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 5 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 31 Oct 2024 10:17:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> Comment-In-Reply-To: ordex <a...@un...> Gerrit-MessageType: comment 

Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email to look at the new patch set (#5). Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... sitnl: replace NLMSG_TAIL macro with noinline function The NLMSG_TAIL macro is confusing gcc when compiling with -O3, leading to warnings like: networking_sitnl.c:143:9: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] 143 | memcpy(RTA_DATA(rta), data, alen); | ^ networking_sitnl.c:101:21: note: at offset [72, 88] into destination object ‘n’ of size 16 101 | struct nlmsghdr n; | ^ Replacing the macro with a function is also not effective because gcc will inline it and get confused again. The only way out is to write a function that never gets inline'd and replace the macro with it. Tested on linux with gcc and clang. Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/networking_sitnl.c 1 file changed, 21 insertions(+), 8 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/88/788/5 diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index f53f5ee..6b750e8 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -52,21 +52,34 @@ } \ } -#define NLMSG_TAIL(nmsg) \ - ((struct rtattr *)(((uint8_t *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len))) - #define SITNL_NEST(_msg, _max_size, _attr) \ ({ \ - struct rtattr *_nest = NLMSG_TAIL(_msg); \ + struct rtattr *_nest = sitnl_nlmsg_tail(_msg); \ SITNL_ADDATTR(_msg, _max_size, _attr, NULL, 0); \ _nest; \ }) -#define SITNL_NEST_END(_msg, _nest) \ - { \ - _nest->rta_len = (void *)NLMSG_TAIL(_msg) - (void *)_nest; \ +#define SITNL_NEST_END(_msg, _nest) \ + { \ + _nest->rta_len = (void *)sitnl_nlmsg_tail(_msg) - (void *)_nest; \ } +/* This function was originally implemented as a macro, but compiling with + * gcc and -O3 was getting confused about the math and thus raising + * security warnings on subsequent memcpy() calls. + * + * Converting the macro to a function was not enough, because gcc was still + * inlining it and falling in the same math trap. + * + * The only way out to avoid any warning/error is to force the function to + * not be inline'd. + */ +static __attribute__ ((noinline)) void * +sitnl_nlmsg_tail(const struct nlmsghdr *nlh) +{ + return (unsigned char *)nlh + NLMSG_ALIGN(nlh->nlmsg_len); +} + /** * Generic address data structure used to pass addresses and prefixes as * argument to AF family agnostic functions @@ -130,7 +143,7 @@ return -EMSGSIZE; } - rta = NLMSG_TAIL(n); + rta = sitnl_nlmsg_tail(n); rta->rta_type = type; rta->rta_len = len; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 5 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset 

Attention is currently required from: flichtenheld, plaisthos. ordex has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/788?usp=email ) Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... Patch Set 4: (1 comment) Commit Message: http://gerrit.openvpn.net/c/openvpn/+/788/comment/23b0d192_044e4c4d : PS3, Line 19: (Above warnings are critical on Fedora 40 as they are turned into errors) > This is misleading. The warning is turned into an error due to -Werror, not due to new GCC version. my intent was to say that this is turned into an error on Fedora by default - the reason resides in Fedora. But I can mention -Werror and drop Fedora. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 4 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 31 Oct 2024 10:06:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> Gerrit-MessageType: comment 

Attention is currently required from: flichtenheld, ordex, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review+2 by flichtenheld The change is no longer submittable: Code-Review and checks~ChecksSubmitRule are unsatisfied now. Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... sitnl: replace NLMSG_TAIL macro with noinline function The NLMSG_TAIL macro is confusing gcc when compiling with -O3, leading to warnings like: networking_sitnl.c:143:9: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] 143 | memcpy(RTA_DATA(rta), data, alen); | ^ networking_sitnl.c:101:21: note: at offset [72, 88] into destination object ‘n’ of size 16 101 | struct nlmsghdr n; | ^ (Above warnings are critical on Fedora 40 as they are turned into errors) Replacing the macro with a function is also not effective because gcc will inline it and get confused again. The only way out is to write a function that never gets inline'd and replace the macro with it. Tested on linux with gcc and clang. Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/networking_sitnl.c 1 file changed, 21 insertions(+), 8 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/88/788/4 diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index f53f5ee..6b750e8 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -52,21 +52,34 @@ } \ } -#define NLMSG_TAIL(nmsg) \ - ((struct rtattr *)(((uint8_t *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len))) - #define SITNL_NEST(_msg, _max_size, _attr) \ ({ \ - struct rtattr *_nest = NLMSG_TAIL(_msg); \ + struct rtattr *_nest = sitnl_nlmsg_tail(_msg); \ SITNL_ADDATTR(_msg, _max_size, _attr, NULL, 0); \ _nest; \ }) -#define SITNL_NEST_END(_msg, _nest) \ - { \ - _nest->rta_len = (void *)NLMSG_TAIL(_msg) - (void *)_nest; \ +#define SITNL_NEST_END(_msg, _nest) \ + { \ + _nest->rta_len = (void *)sitnl_nlmsg_tail(_msg) - (void *)_nest; \ } +/* This function was originally implemented as a macro, but compiling with + * gcc and -O3 was getting confused about the math and thus raising + * security warnings on subsequent memcpy() calls. + * + * Converting the macro to a function was not enough, because gcc was still + * inlining it and falling in the same math trap. + * + * The only way out to avoid any warning/error is to force the function to + * not be inline'd. + */ +static __attribute__ ((noinline)) void * +sitnl_nlmsg_tail(const struct nlmsghdr *nlh) +{ + return (unsigned char *)nlh + NLMSG_ALIGN(nlh->nlmsg_len); +} + /** * Generic address data structure used to pass addresses and prefixes as * argument to AF family agnostic functions @@ -130,7 +143,7 @@ return -EMSGSIZE; } - rta = NLMSG_TAIL(n); + rta = sitnl_nlmsg_tail(n); rta->rta_type = type; rta->rta_len = len; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 4 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ordex <a...@un...> Gerrit-MessageType: newpatchset 

Attention is currently required from: ordex, plaisthos. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/788?usp=email ) Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... Patch Set 3: Code-Review+2 (2 comments) Commit Message: http://gerrit.openvpn.net/c/openvpn/+/788/comment/cc4a5a68_3ea32735 : PS3, Line 19: (Above warnings are critical on Fedora 40 as they are turned into errors) This is misleading. The warning is turned into an error due to -Werror, not due to new GCC version. Patchset: PS3: Patch fixes the issue. Commit message is not completely right, but we might also fix that on submission. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 3 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: ordex <a...@un...> Gerrit-Comment-Date: Wed, 30 Oct 2024 13:56:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment 

Meeting summary for 30 October 2024: * *Updated: DCO and Linux upstreaming, API change* /Upstreaming DCO to Linux is proceeding, it is in review stage at the moment./ /ordex sent in*patchset version 11*. Awaiting feedback./ * *Updated: DCO windows multi-peer* /Kernel-side stuff looks to be mostly done. Currently look into user-space stuff and some proper locking and memory management (refcounters etc)./ * *Updated: multi-socket patch series* /4 preparation work patches were merged. Now it's on to the real patch series./ * *Updated: data format v3 / epoch data keys* /plaisthos has written a new draft for new key handling for the data channel based on discussions in Karlsruhe./ /See​https://github.com/OpenVPN/openvpn-rfc/pull/5 <https://github.com/OpenVPN/openvpn-rfc/pull/5>./ /The current state is that the work for it is done but not yet sent in - plaisthos indicates he wants to test and implement it with openvpn3 before submitting it to openvpn2./ As always you're welcome to join at #openvpn-meeting on Libera IRC network every Wednesday at 14:00 Central European Time. Kind regards, Johan Draaisma 

Attention is currently required from: flichtenheld, ordex, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Code-Review-1 by flichtenheld Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... sitnl: replace NLMSG_TAIL macro with noinline function The NLMSG_TAIL macro is confusing gcc when compiling with -O3, leading to warnings like: networking_sitnl.c:143:9: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] 143 | memcpy(RTA_DATA(rta), data, alen); | ^ networking_sitnl.c:101:21: note: at offset [72, 88] into destination object ‘n’ of size 16 101 | struct nlmsghdr n; | ^ (Above warnings are critical on Fedora 40 as they are turned into errors) Replacing the macro with a function is also not effective because gcc will inline it and get confused again. The only way out is to write a function that never gets inline'd and replace the macro with it. Tested on linux with gcc and clang. Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/networking_sitnl.c 1 file changed, 11 insertions(+), 8 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/88/788/3 diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index f53f5ee..daa0999 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -52,21 +52,24 @@ } \ } -#define NLMSG_TAIL(nmsg) \ - ((struct rtattr *)(((uint8_t *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len))) - #define SITNL_NEST(_msg, _max_size, _attr) \ ({ \ - struct rtattr *_nest = NLMSG_TAIL(_msg); \ + struct rtattr *_nest = sitnl_nlmsg_tail(_msg); \ SITNL_ADDATTR(_msg, _max_size, _attr, NULL, 0); \ _nest; \ }) -#define SITNL_NEST_END(_msg, _nest) \ - { \ - _nest->rta_len = (void *)NLMSG_TAIL(_msg) - (void *)_nest; \ +#define SITNL_NEST_END(_msg, _nest) \ + { \ + _nest->rta_len = (void *)sitnl_nlmsg_tail(_msg) - (void *)_nest; \ } +static __attribute__ ((noinline)) void * +sitnl_nlmsg_tail(const struct nlmsghdr *nlh) +{ + return (unsigned char *)nlh + NLMSG_ALIGN(nlh->nlmsg_len); +} + /** * Generic address data structure used to pass addresses and prefixes as * argument to AF family agnostic functions @@ -130,7 +133,7 @@ return -EMSGSIZE; } - rta = NLMSG_TAIL(n); + rta = sitnl_nlmsg_tail(n); rta->rta_type = type; rta->rta_len = len; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 3 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ordex <a...@un...> Gerrit-MessageType: newpatchset 

Attention is currently required from: ordex, plaisthos. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/788?usp=email ) Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... Patch Set 2: Code-Review-1 (1 comment) Patchset: PS2: Please fix uncrustify error -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: ordex <a...@un...> Gerrit-Comment-Date: Tue, 29 Oct 2024 17:20:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment 

Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email to look at the new patch set (#2). Change subject: sitnl: replace NLMSG_TAIL macro with noinline function ...................................................................... sitnl: replace NLMSG_TAIL macro with noinline function The NLMSG_TAIL macro is confusing gcc when compiling with -O3, leading to warnings like: networking_sitnl.c:143:9: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] 143 | memcpy(RTA_DATA(rta), data, alen); | ^ networking_sitnl.c:101:21: note: at offset [72, 88] into destination object ‘n’ of size 16 101 | struct nlmsghdr n; | ^ (Above warnings are critical on Fedora 40 as they are turned into errors) Replacing the macro with a function is also not effective because gcc will inline it and get confused again. The only way out is to write a function that never gets inline'd and replace the macro with it. Tested on linux with gcc and clang. Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/networking_sitnl.c 1 file changed, 11 insertions(+), 8 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/88/788/2 diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index f53f5ee..3d83a6b 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -52,21 +52,24 @@ } \ } -#define NLMSG_TAIL(nmsg) \ - ((struct rtattr *)(((uint8_t *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len))) - #define SITNL_NEST(_msg, _max_size, _attr) \ ({ \ - struct rtattr *_nest = NLMSG_TAIL(_msg); \ + struct rtattr *_nest = sitnl_nlmsg_tail(_msg); \ SITNL_ADDATTR(_msg, _max_size, _attr, NULL, 0); \ _nest; \ }) -#define SITNL_NEST_END(_msg, _nest) \ - { \ - _nest->rta_len = (void *)NLMSG_TAIL(_msg) - (void *)_nest; \ +#define SITNL_NEST_END(_msg, _nest)	\ + {	\ + _nest->rta_len = (void *)sitnl_nlmsg_tail(_msg) - (void *)_nest;\ } +static __attribute__ ((noinline)) void * +sitnl_nlmsg_tail(const struct nlmsghdr *nlh) +{ +	return (unsigned char *)nlh + NLMSG_ALIGN(nlh->nlmsg_len); +} + /** * Generic address data structure used to pass addresses and prefixes as * argument to AF family agnostic functions @@ -130,7 +133,7 @@ return -EMSGSIZE; } - rta = NLMSG_TAIL(n); + rta = sitnl_nlmsg_tail(n); rta->rta_type = type; rta->rta_len = len; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset 

From: Antonio Quartulli <an...@ma...> The NLMSG_TAIL macro never had any reason to exist, because libnl already provides a function doing exactly the same: nlmsg_tail(). Moreover, this macro was found to confuse gcc when compiling with -O3, which would result in subsequent warnings like: networking_sitnl.c:143:9: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] 143 | memcpy(RTA_DATA(rta), data, alen); | ^ networking_sitnl.c:101:21: note: at offset [72, 88] into destination object ‘n’ of size 16 101 | struct nlmsghdr n; | ^ (Above warnings are critical on Fedora 40 as they are turned into errors) Delete the macro, replace it with nlmsg_tail() and get rid of the warnings. Signed-off-by: Antonio Quartulli <an...@ma...> --- src/openvpn/networking_sitnl.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index f53f5ee9..8eeab72a 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -40,6 +40,7 @@ #include <sys/socket.h> #include <linux/netlink.h> #include <linux/rtnetlink.h> +#include <netlink/msg.h> #define SNDBUF_SIZE (1024 * 2) #define RCVBUF_SIZE (1024 * 4) @@ -52,19 +53,16 @@ } \ } -#define NLMSG_TAIL(nmsg) \ - ((struct rtattr *)(((uint8_t *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len))) - #define SITNL_NEST(_msg, _max_size, _attr) \ ({ \ - struct rtattr *_nest = NLMSG_TAIL(_msg); \ + struct rtattr *_nest = nlmsg_tail(_msg); \ SITNL_ADDATTR(_msg, _max_size, _attr, NULL, 0); \ _nest; \ }) #define SITNL_NEST_END(_msg, _nest) \ { \ - _nest->rta_len = (void *)NLMSG_TAIL(_msg) - (void *)_nest; \ + _nest->rta_len = (void *)nlmsg_tail(_msg) - (void *)_nest; \ } /** @@ -130,7 +128,7 @@ sitnl_addattr(struct nlmsghdr *n, int maxlen, int type, const void *data, return -EMSGSIZE; } - rta = NLMSG_TAIL(n); + rta = nlmsg_tail(n); rta->rta_type = type; rta->rta_len = len; -- 2.45.2 

Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email to review the following change. Change subject: sitnl: replace NLMSG_TAIL macro with nlmsg_tail() function ...................................................................... sitnl: replace NLMSG_TAIL macro with nlmsg_tail() function The NLMSG_TAIL macro never had any reason to exist, because libnl already provides a function doing exactly the same: nlmsg_tail(). Moreover, this macro was found to confuse gcc when compiling with -O3, which would result in subsequent warnings like: networking_sitnl.c:143:9: warning: writing 4 bytes into a region of size 0 [-Wstringop-overflow=] 143 | memcpy(RTA_DATA(rta), data, alen); | ^ networking_sitnl.c:101:21: note: at offset [72, 88] into destination object ‘n’ of size 16 101 | struct nlmsghdr n; | ^ (Above warnings are critical on Fedora 40 as they are turned into errors) Delete the macro, replace it with nlmsg_tail() and get rid of the warnings. Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Signed-off-by: Antonio Quartulli <an...@ma...> --- M src/openvpn/networking_sitnl.c 1 file changed, 4 insertions(+), 6 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/88/788/1 diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index f53f5ee..8eeab72 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -40,6 +40,7 @@ #include <sys/socket.h> #include <linux/netlink.h> #include <linux/rtnetlink.h> +#include <netlink/msg.h> #define SNDBUF_SIZE (1024 * 2) #define RCVBUF_SIZE (1024 * 4) @@ -52,19 +53,16 @@ } \ } -#define NLMSG_TAIL(nmsg) \ - ((struct rtattr *)(((uint8_t *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len))) - #define SITNL_NEST(_msg, _max_size, _attr) \ ({ \ - struct rtattr *_nest = NLMSG_TAIL(_msg); \ + struct rtattr *_nest = nlmsg_tail(_msg); \ SITNL_ADDATTR(_msg, _max_size, _attr, NULL, 0); \ _nest; \ }) #define SITNL_NEST_END(_msg, _nest) \ { \ - _nest->rta_len = (void *)NLMSG_TAIL(_msg) - (void *)_nest; \ + _nest->rta_len = (void *)nlmsg_tail(_msg) - (void *)_nest; \ } /** @@ -130,7 +128,7 @@ return -EMSGSIZE; } - rta = NLMSG_TAIL(n); + rta = nlmsg_tail(n); rta->rta_type = type; rta->rta_len = len; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/788?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I9306a590a10a7d5cba32abe06d269494fec41ba6 Gerrit-Change-Number: 788 Gerrit-PatchSet: 1 Gerrit-Owner: ordex <a...@un...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange 

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/787?usp=email ) Change subject: Refuse clients if username or password is longer than USER_PASS_LEN ...................................................................... Refuse clients if username or password is longer than USER_PASS_LEN When OpenVPN is compiled without PKCS11 support USER_PASS_LEN is 128 bytes. If we encounter a username larger than this length, we would only read the 2 bytes length header of the username/password. We did then also NOT skip the username or password field meaning that we would continue reading the rest of the packet at the wrong offset and get garbage results like not having peerinfo and then rejecting a client because of no common cipher or missing data v2 support. This will tell the client that username/password is too regardless of whether password/username authentication is used. This way we do not leak if username/password authentication is active. To reproduce this issue have the server compiled with a USER_PASS_LEN set to 128 (e.g. without pkcs11 or manually adjusting the define) and have the client with a larger USER_PASS_LEN to actually be able to send the larger password. The server must also be set to use only certificate authentication while the client must use certificates and auth-user-pass because otherwise the user/pass verification will reject the empty credentials. Using the openvpn3 test client with overlong username/password also works. Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29675.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/ssl.c 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index e2be614..8040e7b 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1835,20 +1835,33 @@ return true; } -static bool +/** + * Read a string that is encoded as a 2 byte header with the length from the + * buffer \c buf. Will return the non-negative value if reading was successful. + * The returned value will include the trailing 0 byte. + * + * If the message is over the capacity or could not be read + * it will return the negative length that was in the + * header and try to skip the string. If the string cannot be skipped, the + * buf will stay at the current position or position + 2 + */ +static int read_string(struct buffer *buf, char *str, const unsigned int capacity) { const int len = buf_read_u16(buf); if (len < 1 || len > (int)capacity) { - return false; + buf_advance(buf, len); + + /* will also return 0 for a no string being present */ + return -len; } if (!buf_read(buf, str, len)) { - return false; + return -len; } str[len-1] = '\0'; - return true; + return len; } static char * @@ -2218,8 +2231,6 @@ { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ - bool username_status, password_status; - struct gc_arena gc = gc_new(); char *options; struct user_pass *up = NULL; @@ -2253,7 +2264,7 @@ } /* get options */ - if (!read_string(buf, options, TLS_OPTIONS_LEN)) + if (read_string(buf, options, TLS_OPTIONS_LEN) < 0) { msg(D_TLS_ERRORS, "TLS Error: Failed to read required OCC options string"); goto error; @@ -2266,8 +2277,8 @@ * peer_info data which follows behind */ ALLOC_OBJ_CLEAR_GC(up, struct user_pass, &gc); - username_status = read_string(buf, up->username, USER_PASS_LEN); - password_status = read_string(buf, up->password, USER_PASS_LEN); + int username_len = read_string(buf, up->username, USER_PASS_LEN); + int password_len = read_string(buf, up->password, USER_PASS_LEN); /* get peer info from control channel */ free(multi->peer_info); @@ -2290,10 +2301,21 @@ multi->remote_ciphername = string_alloc("none", NULL); } - if (tls_session_user_pass_enabled(session)) + if (username_len < 0 || password_len < 0) + { + msg(D_TLS_ERRORS, "TLS Error: Username (%d) or password (%d) too long", + abs(username_len), abs(password_len)); + auth_set_client_reason(multi, "Username or password is too long. " + "Maximum length is 128 bytes"); + + /* treat the same as failed username/password and do not error + * out (goto error) to sent an AUTH_FAILED back to the client */ + ks->authenticated = KS_AUTH_FALSE; + } + else if (tls_session_user_pass_enabled(session)) { /* Perform username/password authentication */ - if (!username_status || !password_status) + if (!username_len || !password_len) { CLEAR(*up); if (!(session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL)) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 3 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged 

cron2 has uploaded a new patch set (#3) to the change originally created by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/787?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: Refuse clients if username or password is longer than USER_PASS_LEN ...................................................................... Refuse clients if username or password is longer than USER_PASS_LEN When OpenVPN is compiled without PKCS11 support USER_PASS_LEN is 128 bytes. If we encounter a username larger than this length, we would only read the 2 bytes length header of the username/password. We did then also NOT skip the username or password field meaning that we would continue reading the rest of the packet at the wrong offset and get garbage results like not having peerinfo and then rejecting a client because of no common cipher or missing data v2 support. This will tell the client that username/password is too regardless of whether password/username authentication is used. This way we do not leak if username/password authentication is active. To reproduce this issue have the server compiled with a USER_PASS_LEN set to 128 (e.g. without pkcs11 or manually adjusting the define) and have the client with a larger USER_PASS_LEN to actually be able to send the larger password. The server must also be set to use only certificate authentication while the client must use certificates and auth-user-pass because otherwise the user/pass verification will reject the empty credentials. Using the openvpn3 test client with overlong username/password also works. Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29675.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/ssl.c 1 file changed, 33 insertions(+), 11 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/87/787/3 diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index e2be614..8040e7b 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1835,20 +1835,33 @@ return true; } -static bool +/** + * Read a string that is encoded as a 2 byte header with the length from the + * buffer \c buf. Will return the non-negative value if reading was successful. + * The returned value will include the trailing 0 byte. + * + * If the message is over the capacity or could not be read + * it will return the negative length that was in the + * header and try to skip the string. If the string cannot be skipped, the + * buf will stay at the current position or position + 2 + */ +static int read_string(struct buffer *buf, char *str, const unsigned int capacity) { const int len = buf_read_u16(buf); if (len < 1 || len > (int)capacity) { - return false; + buf_advance(buf, len); + + /* will also return 0 for a no string being present */ + return -len; } if (!buf_read(buf, str, len)) { - return false; + return -len; } str[len-1] = '\0'; - return true; + return len; } static char * @@ -2218,8 +2231,6 @@ { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ - bool username_status, password_status; - struct gc_arena gc = gc_new(); char *options; struct user_pass *up = NULL; @@ -2253,7 +2264,7 @@ } /* get options */ - if (!read_string(buf, options, TLS_OPTIONS_LEN)) + if (read_string(buf, options, TLS_OPTIONS_LEN) < 0) { msg(D_TLS_ERRORS, "TLS Error: Failed to read required OCC options string"); goto error; @@ -2266,8 +2277,8 @@ * peer_info data which follows behind */ ALLOC_OBJ_CLEAR_GC(up, struct user_pass, &gc); - username_status = read_string(buf, up->username, USER_PASS_LEN); - password_status = read_string(buf, up->password, USER_PASS_LEN); + int username_len = read_string(buf, up->username, USER_PASS_LEN); + int password_len = read_string(buf, up->password, USER_PASS_LEN); /* get peer info from control channel */ free(multi->peer_info); @@ -2290,10 +2301,21 @@ multi->remote_ciphername = string_alloc("none", NULL); } - if (tls_session_user_pass_enabled(session)) + if (username_len < 0 || password_len < 0) + { + msg(D_TLS_ERRORS, "TLS Error: Username (%d) or password (%d) too long", + abs(username_len), abs(password_len)); + auth_set_client_reason(multi, "Username or password is too long. " + "Maximum length is 128 bytes"); + + /* treat the same as failed username/password and do not error + * out (goto error) to sent an AUTH_FAILED back to the client */ + ks->authenticated = KS_AUTH_FALSE; + } + else if (tls_session_user_pass_enabled(session)) { /* Perform username/password authentication */ - if (!username_status || !password_status) + if (!username_len || !password_len) { CLEAR(*up); if (!(session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL)) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 3 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset 

Tested this on the local t_server installation, sending overlong usernames from a client with --enable-pkcs11 - without the patch, there's no crashes or anything (= not a security relevant bug), but the server gets confused and does not send a proper TLS FAIL back. With the patch, the server will send a proper AUTH FAIL 2024-10-27 10:34:48 AUTH: Received control message: AUTH_FAILED,Username or password is too long. Maximum length is 128 bytes We have a bit more work to do (the client is not properly rejecting overlong usernames either, at least if reading an "--auth-user-pass up.txt" file) - but at least the server side is behaving more nicely now. Your patch has been applied to the master and release/2.6 branch (bugfix). commit a7f80d402fb95df3c58a8fc5d12cdb8f39c37d3e (master) commit b98ff0e7c60c6592a2e8d2c80dfd5999e5d2e65b (release/2.6) Author: Arne Schwabe Date: Mon Oct 28 14:55:04 2024 +0100 Refuse clients if username or password is longer than USER_PASS_LEN Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29675.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

From: Arne Schwabe <ar...@rf...> When OpenVPN is compiled without PKCS11 support USER_PASS_LEN is 128 bytes. If we encounter a username larger than this length, we would only read the 2 bytes length header of the username/password. We did then also NOT skip the username or password field meaning that we would continue reading the rest of the packet at the wrong offset and get garbage results like not having peerinfo and then rejecting a client because of no common cipher or missing data v2 support. This will tell the client that username/password is too regardless of whether password/username authentication is used. This way we do not leak if username/password authentication is active. To reproduce this issue have the server compiled with a USER_PASS_LEN set to 128 (e.g. without pkcs11 or manually adjusting the define) and have the client with a larger USER_PASS_LEN to actually be able to send the larger password. The server must also be set to use only certificate authentication while the client must use certificates and auth-user-pass because otherwise the user/pass verification will reject the empty credentials. Using the openvpn3 test client with overlong username/password also works. Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/787 This mail reflects revision 2 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index e2be614..8040e7b 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1835,20 +1835,33 @@ return true; } -static bool +/** + * Read a string that is encoded as a 2 byte header with the length from the + * buffer \c buf. Will return the non-negative value if reading was successful. + * The returned value will include the trailing 0 byte. + * + * If the message is over the capacity or could not be read + * it will return the negative length that was in the + * header and try to skip the string. If the string cannot be skipped, the + * buf will stay at the current position or position + 2 + */ +static int read_string(struct buffer *buf, char *str, const unsigned int capacity) { const int len = buf_read_u16(buf); if (len < 1 || len > (int)capacity) { - return false; + buf_advance(buf, len); + + /* will also return 0 for a no string being present */ + return -len; } if (!buf_read(buf, str, len)) { - return false; + return -len; } str[len-1] = '\0'; - return true; + return len; } static char * @@ -2218,8 +2231,6 @@ { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ - bool username_status, password_status; - struct gc_arena gc = gc_new(); char *options; struct user_pass *up = NULL; @@ -2253,7 +2264,7 @@ } /* get options */ - if (!read_string(buf, options, TLS_OPTIONS_LEN)) + if (read_string(buf, options, TLS_OPTIONS_LEN) < 0) { msg(D_TLS_ERRORS, "TLS Error: Failed to read required OCC options string"); goto error; @@ -2266,8 +2277,8 @@ * peer_info data which follows behind */ ALLOC_OBJ_CLEAR_GC(up, struct user_pass, &gc); - username_status = read_string(buf, up->username, USER_PASS_LEN); - password_status = read_string(buf, up->password, USER_PASS_LEN); + int username_len = read_string(buf, up->username, USER_PASS_LEN); + int password_len = read_string(buf, up->password, USER_PASS_LEN); /* get peer info from control channel */ free(multi->peer_info); @@ -2290,10 +2301,21 @@ multi->remote_ciphername = string_alloc("none", NULL); } - if (tls_session_user_pass_enabled(session)) + if (username_len < 0 || password_len < 0) + { + msg(D_TLS_ERRORS, "TLS Error: Username (%d) or password (%d) too long", + abs(username_len), abs(password_len)); + auth_set_client_reason(multi, "Username or password is too long. " + "Maximum length is 128 bytes"); + + /* treat the same as failed username/password and do not error + * out (goto error) to sent an AUTH_FAILED back to the client */ + ks->authenticated = KS_AUTH_FALSE; + } + else if (tls_session_user_pass_enabled(session)) { /* Perform username/password authentication */ - if (!username_status || !password_status) + if (!username_len || !password_len) { CLEAR(*up); if (!(session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL)) 

Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/787?usp=email ) Change subject: Refuse clients if username or password is longer than USER_PASS_LEN ...................................................................... Patch Set 2: Code-Review+2 (1 comment) File src/openvpn/ssl.c: http://gerrit.openvpn.net/c/openvpn/+/787/comment/2171c946_e254f51c : PS1, Line 2284: abs(username_len), abs(password_len)); > Yeah sounds good. Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Mon, 28 Oct 2024 13:54:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: plaisthos <arn...@rf...> Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment 

Attention is currently required from: cron2, flichtenheld. Hello cron2, flichtenheld, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Code-Review-1 by cron2 Change subject: Refuse clients if username or password is longer than USER_PASS_LEN ...................................................................... Refuse clients if username or password is longer than USER_PASS_LEN When OpenVPN is compiled without PKCS11 support USER_PASS_LEN is 128 bytes. If we encounter a username larger than this length, we would only read the 2 bytes length header of the username/password. We did then also NOT skip the username or password field meaning that we would continue reading the rest of the packet at the wrong offset and get garbage results like not having peerinfo and then rejecting a client because of no common cipher or missing data v2 support. This will tell the client that username/password is too regardless of whether password/username authentication is used. This way we do not leak if username/password authentication is active. To reproduce this issue have the server compiled with a USER_PASS_LEN set to 128 (e.g. without pkcs11 or manually adjusting the define) and have the client with a larger USER_PASS_LEN to actually be able to send the larger password. The server must also be set to use only certificate authentication while the client must use certificates and auth-user-pass because otherwise the user/pass verification will reject the empty credentials. Using the openvpn3 test client with overlong username/password also works. Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Signed-off-by: Arne Schwabe <ar...@rf...> --- M src/openvpn/ssl.c 1 file changed, 33 insertions(+), 11 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/87/787/2 diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index e2be614..8040e7b 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1835,20 +1835,33 @@ return true; } -static bool +/** + * Read a string that is encoded as a 2 byte header with the length from the + * buffer \c buf. Will return the non-negative value if reading was successful. + * The returned value will include the trailing 0 byte. + * + * If the message is over the capacity or could not be read + * it will return the negative length that was in the + * header and try to skip the string. If the string cannot be skipped, the + * buf will stay at the current position or position + 2 + */ +static int read_string(struct buffer *buf, char *str, const unsigned int capacity) { const int len = buf_read_u16(buf); if (len < 1 || len > (int)capacity) { - return false; + buf_advance(buf, len); + + /* will also return 0 for a no string being present */ + return -len; } if (!buf_read(buf, str, len)) { - return false; + return -len; } str[len-1] = '\0'; - return true; + return len; } static char * @@ -2218,8 +2231,6 @@ { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ - bool username_status, password_status; - struct gc_arena gc = gc_new(); char *options; struct user_pass *up = NULL; @@ -2253,7 +2264,7 @@ } /* get options */ - if (!read_string(buf, options, TLS_OPTIONS_LEN)) + if (read_string(buf, options, TLS_OPTIONS_LEN) < 0) { msg(D_TLS_ERRORS, "TLS Error: Failed to read required OCC options string"); goto error; @@ -2266,8 +2277,8 @@ * peer_info data which follows behind */ ALLOC_OBJ_CLEAR_GC(up, struct user_pass, &gc); - username_status = read_string(buf, up->username, USER_PASS_LEN); - password_status = read_string(buf, up->password, USER_PASS_LEN); + int username_len = read_string(buf, up->username, USER_PASS_LEN); + int password_len = read_string(buf, up->password, USER_PASS_LEN); /* get peer info from control channel */ free(multi->peer_info); @@ -2290,10 +2301,21 @@ multi->remote_ciphername = string_alloc("none", NULL); } - if (tls_session_user_pass_enabled(session)) + if (username_len < 0 || password_len < 0) + { + msg(D_TLS_ERRORS, "TLS Error: Username (%d) or password (%d) too long", + abs(username_len), abs(password_len)); + auth_set_client_reason(multi, "Username or password is too long. " + "Maximum length is 128 bytes"); + + /* treat the same as failed username/password and do not error + * out (goto error) to sent an AUTH_FAILED back to the client */ + ks->authenticated = KS_AUTH_FALSE; + } + else if (tls_session_user_pass_enabled(session)) { /* Perform username/password authentication */ - if (!username_status || !password_status) + if (!username_len || !password_len) { CLEAR(*up); if (!(session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL)) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset 

Attention is currently required from: cron2, flichtenheld. plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/787?usp=email ) Change subject: Refuse clients if username or password is > USER_PASS_LEN ...................................................................... Patch Set 1: (1 comment) File src/openvpn/ssl.c: http://gerrit.openvpn.net/c/openvpn/+/787/comment/932858d7_a579b6b4 : PS1, Line 2284: abs(username_len), abs(password_len)); > You have two messages there - the "TLS INFO:" which is always printed, and only has a negative numbe […] Yeah sounds good. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Mon, 28 Oct 2024 13:12:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: plaisthos <arn...@rf...> Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment 

Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/787?usp=email ) Change subject: Refuse clients if username or password is > USER_PASS_LEN ...................................................................... Patch Set 1: (1 comment) File src/openvpn/ssl.c: http://gerrit.openvpn.net/c/openvpn/+/787/comment/144948ce_63eadfa8 : PS1, Line 2284: abs(username_len), abs(password_len)); > I think a negative length is more alarming and confusing that a positive one. […] You have two messages there - the "TLS INFO:" which is always printed, and only has a negative number if one of the strings is too long. This is what I'm talking about. The second message is the "TLS Error:" which is only printed in case of overflow - the abs() is reasonable there. So I'd suggest to remove the "TLS INFO:" message, because in case of errors it's just duplicate information, and in case of non-errors, it's log noise. No? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Mon, 28 Oct 2024 10:09:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: plaisthos <arn...@rf...> Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment 

Attention is currently required from: cron2, flichtenheld. plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/787?usp=email ) Change subject: Refuse clients if username or password is > USER_PASS_LEN ...................................................................... Patch Set 1: (1 comment) File src/openvpn/ssl.c: http://gerrit.openvpn.net/c/openvpn/+/787/comment/801fc40c_ea3a9703 : PS1, Line 2284: abs(username_len), abs(password_len)); > I suggest printing the variables without `abs()` so the log is clear *if* there is an overrun - in m […] I think a negative length is more alarming and confusing that a positive one. If you remove the abs() one of the number will be always negative. Since it errors out now anyway and of them is too long anyway, it is a larger one. I just didn't want to do all the if/else cases and different message for password too long and username too long. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Sun, 27 Oct 2024 15:17:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment 

Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/787?usp=email ) Change subject: Refuse clients if username or password is > USER_PASS_LEN ...................................................................... Patch Set 1: Code-Review-1 (1 comment) Patchset: PS1: Okay, my client was not compiled the way I thought - so with --enable-pkcs11, sending a long username to an unmodified server yields ``` Oct 27 10:30:59 gentoo tun-udp-p2mp-global-authpam[14201]: 194.97.140.21:36736 TLS Error: Auth Username/Password was not provided by peer Oct 27 10:30:59 gentoo tun-udp-p2mp-global-authpam[14201]: 194.97.140.21:36736 TLS Error: TLS handshake failed `` and with the patch it does a proper ``` Oct 27 10:34:48 gentoo tun-udp-p2mp-global-authpam[15712]: 194.97.140.21:60127 TLS INFO: Username (-230) or password (14) long ... Oct 27 10:34:48 gentoo tun-udp-p2mp-global-authpam[15712]: 194.97.140.21:60127 TLS Error: Username (230) or password (14) too long ``` and the client receives ``` 2024-10-27 10:34:48 AUTH: Received control message: AUTH_FAILED,Username or password is too long. Maximum length is 128 bytes ``` (the "-" 230 is my doing, I removed the abs() call to more clearly see what is being returned). Upgrading the patch to "-1" ;-) - I think the "TLS INFO:" line clould either be removed (because it's duplicating the TLS Error: message later) or the `abs()` should go, and the double space before ` long`) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Gerrit-Change-Number: 787 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Sun, 27 Oct 2024 09:41:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment 

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/786?usp=email ) Change subject: t_server_null: use wait instead of marker files ...................................................................... t_server_null: use wait instead of marker files By using wait in a more inventive way we can avoid using a marker file to detect the "server could not be killed gracefully" situation. Change-Id: Ib385080e1dd1c3046c54e6267db8aa7d5c09e2fb Signed-off-by: Samuli Seppänen <sam...@gm...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29664.html Signed-off-by: Gert Doering <ge...@gr...> --- M tests/t_server_null.sh M tests/t_server_null_server.sh 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/t_server_null.sh b/tests/t_server_null.sh index 3c0fc4b..74ffd52 100755 --- a/tests/t_server_null.sh +++ b/tests/t_server_null.sh @@ -62,6 +62,8 @@ mkdir $t_server_null_logdir "${srcdir}/t_server_null_server.sh" & +T_SERVER_NULL_SERVER_PID=$! + "${srcdir}/t_server_null_client.sh" retval=$? @@ -69,11 +71,9 @@ # that this script does not exit before all --dev null servers are dead and # their network interfaces are gone. Otherwise t_client.sh will fail because # pre and post ifconfig output does not match. -wait +wait $T_SERVER_NULL_SERVER_PID -. ./t_server_null_default.rc - -if [ -e $SERVER_KILL_FAIL_FILE ]; then +if [ $? -ne 0 ]; then exit 1 else exit $retval diff --git a/tests/t_server_null_server.sh b/tests/t_server_null_server.sh index ab01dd2..acf8479 100755 --- a/tests/t_server_null_server.sh +++ b/tests/t_server_null_server.sh @@ -37,8 +37,9 @@ # Load local configuration, if any test -r ./t_server_null.rc && . ./t_server_null.rc -# Remove server kill failure marker file, if any -rm -f $SERVER_KILL_FAIL_FILE +# We can't exit immediately on the first failure as that could leave processes +# lying around. +retval=0 # Launch test servers for SUF in $TEST_SERVER_LIST @@ -81,7 +82,6 @@ # Make sure that the server processes are truly dead before exiting. If a # server process does not exit in 15 seconds assume it never will, move on and # hope for the best. - echo "Waiting for servers to exit" for PID_FILE in $server_pid_files do @@ -111,6 +111,8 @@ echo "ERROR: had to send SIGKILL to server ${SERVER_NAME} with pid ${SERVER_PID}!" echo "Tail of server log:" tail -n 20 "${t_server_null_logdir}/${SERVER_NAME}.log" - touch $SERVER_KILL_FAIL_FILE + retval=1 fi done + +exit $retval -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/786?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ib385080e1dd1c3046c54e6267db8aa7d5c09e2fb Gerrit-Change-Number: 786 Gerrit-PatchSet: 2 Gerrit-Owner: mattock <sa...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged 

cron2 has uploaded a new patch set (#2) to the change originally created by mattock. ( http://gerrit.openvpn.net/c/openvpn/+/786?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: t_server_null: use wait instead of marker files ...................................................................... t_server_null: use wait instead of marker files By using wait in a more inventive way we can avoid using a marker file to detect the "server could not be killed gracefully" situation. Change-Id: Ib385080e1dd1c3046c54e6267db8aa7d5c09e2fb Signed-off-by: Samuli Seppänen <sam...@gm...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29664.html Signed-off-by: Gert Doering <ge...@gr...> --- M tests/t_server_null.sh M tests/t_server_null_server.sh 2 files changed, 10 insertions(+), 8 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/86/786/2 diff --git a/tests/t_server_null.sh b/tests/t_server_null.sh index 3c0fc4b..74ffd52 100755 --- a/tests/t_server_null.sh +++ b/tests/t_server_null.sh @@ -62,6 +62,8 @@ mkdir $t_server_null_logdir "${srcdir}/t_server_null_server.sh" & +T_SERVER_NULL_SERVER_PID=$! + "${srcdir}/t_server_null_client.sh" retval=$? @@ -69,11 +71,9 @@ # that this script does not exit before all --dev null servers are dead and # their network interfaces are gone. Otherwise t_client.sh will fail because # pre and post ifconfig output does not match. -wait +wait $T_SERVER_NULL_SERVER_PID -. ./t_server_null_default.rc - -if [ -e $SERVER_KILL_FAIL_FILE ]; then +if [ $? -ne 0 ]; then exit 1 else exit $retval diff --git a/tests/t_server_null_server.sh b/tests/t_server_null_server.sh index ab01dd2..acf8479 100755 --- a/tests/t_server_null_server.sh +++ b/tests/t_server_null_server.sh @@ -37,8 +37,9 @@ # Load local configuration, if any test -r ./t_server_null.rc && . ./t_server_null.rc -# Remove server kill failure marker file, if any -rm -f $SERVER_KILL_FAIL_FILE +# We can't exit immediately on the first failure as that could leave processes +# lying around. +retval=0 # Launch test servers for SUF in $TEST_SERVER_LIST @@ -81,7 +82,6 @@ # Make sure that the server processes are truly dead before exiting. If a # server process does not exit in 15 seconds assume it never will, move on and # hope for the best. - echo "Waiting for servers to exit" for PID_FILE in $server_pid_files do @@ -111,6 +111,8 @@ echo "ERROR: had to send SIGKILL to server ${SERVER_NAME} with pid ${SERVER_PID}!" echo "Tail of server log:" tail -n 20 "${t_server_null_logdir}/${SERVER_NAME}.log" - touch $SERVER_KILL_FAIL_FILE + retval=1 fi done + +exit $retval -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/786?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ib385080e1dd1c3046c54e6267db8aa7d5c09e2fb Gerrit-Change-Number: 786 Gerrit-PatchSet: 2 Gerrit-Owner: mattock <sa...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset