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.

Sign up for GitHub

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

Jump to bottom

Bugfix/ncl 2203 scc windows fix vulnerability in open vpn #3

Merged
merged 24 commits into from
Jan 20, 2025
Merged

Bugfix/ncl 2203 scc windows fix vulnerability in open vpn #3

merged 24 commits into from
Jan 20, 2025

Conversation

rishipal-Sophos
Copy link

Upgrade openvpn 2.6.10 to 2.6.12 to fix the BlackDuck vulnerability

flichtenheld and others added 24 commits March 25, 2024 13:54
@flichtenheld @cron2
samples: Update sample configurations
371cc58 
- Remove compression settings. Not recommended anymore.
- Remove old cipher setting. Replaced by data-ciphers negotiation.
- Add comment how to set data-ciphers for very old clients.
- Remove/reword some old comments. e.g. no need to reference
  OpenVPN 1.x anymore.
- Mention peer-fingerprint alternative.
- comment out "tls-auth" as that is not needed for a bare-bones VPN config
  and needs additional setup.

Github: OpenVPN#511
Change-Id: I1a36651c0dea52259533ffc00bccb9b03bf82e26
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Message-Id: <20240325071320.11348-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28451.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit b0fc10a)
@flichtenheld @cron2
documentation: make section levels consistent
7993084 
Previously the sections "Encryption Options" and
"Data channel cipher negotiation" were on the same
level as "OPTIONS", which makes no sense. Instead
move them and their subsections one level down.

Use ` since that was already in use in section
"Virtual Routing and Forwarding".

Change-Id: Ib5a7f9a978bda5ad58830e43580232660401f66d
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Message-Id: <20240325071520.12513-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28453.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 3fdf5aa)
@schwabe @cron2
Use snprintf instead of sprintf for get_ssl_library_version
11ca69c 
This is avoid a warning/error (when using -Werror) under current macOS
of sprintf:

   __deprecated_msg("This function is provided for compatibility
   reasons only.  Due to security concerns inherent in the design
   of sprintf(3), it is highly recommended that you use snprintf(3)
   instead.")

Change-Id: I3c6fd36eb9daee9244d6dc6d9f22de1c5cf9d039
Signed-off-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240325125052.14135-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28458.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 6a60d1b)
@flichtenheld @cron2
phase2_tcp_server: fix Coverity issue 'Dereference after null check'
5591af1 
As Coverity says:
Either the check against null is unnecessary, or there may be a null
pointer dereference.
In phase2_tcp_server: Pointer is checked against null but then
dereferenced anyway

There is only one caller (link_socket_init_phase2) and it already has
an ASSERT(sig_info). So use that here was well.

v2:
 - fix cleanly by actually asserting that sig_info is defined

Change-Id: I8ef199463d46303129a3f563fd9eace780a58b8a
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Message-Id: <20240325071448.12143-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28452.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit e8c629f)
@schwabe @cron2
Add bracket in fingerprint message and do not warn about missing veri…
e36359a 
…fication

Github: fixes OpenVPN#516

Change-Id: Ia73d53002f4ba2658af18c17cce1b68f79de5781
Signed-off-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240326103853.494572-1-frank@lichtenheld.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28474.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 4b95656)
@flichtenheld @cron2
script-options.rst: Update ifconfig_* variables
ea0d9c7 
- Remove obsolete ifconfig_broadcast. Since this was
  removed in 2.5.0, do not add a removal note but just
  completely remove it.
- Add missing documentation of IPv6 variants for
  ifconfig_pool_* variables.

Github: fixes OpenVPN#527
Change-Id: Ia8c8de6799f0291fc900628fbd06c8a414e741ca
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20240321161623.2794161-1-frank@lichtenheld.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28438.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit a94226c)
@lstipakov @cron2
misc.c: remove unused code
f50c677 
Commit

  3a4fb1 "Ensure --auth-nocache is handled during renegotiation"

has changed the behavior of set_auth_token(), but left unused parameter

  struct user_pass *up

Remove this parameter and amend comments accordingly. Also remove
unused function definition from misc.h.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>

Change-Id: Ic440f2c8d46dfcb5ff41ba2f33bf28bb7286eec4
Message-Id: <20240329103739.28254-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28503.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 4c71e81)
@schwabe @cron2
Replace macos11 with macos14 in github runners
18520e5 
Github's documentation states:  macos-11 label has been deprecated and
will no longer be available after 6/28/2024. Add macos14 which is nowadays
supported instead.

The github macos-14 runner is using the M1 platform with ARM, so this
requires a bit more adjustment of paths.

Change-Id: Ia70f230b2e9a78939d1875395205c8f48c4944b7
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240502122231.672-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/search?l=mid&q=20240502122231.672-1-gert@greenie.muc.de
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 02f0845)
@schwabe @cron2
Only run coverity scan in OpenVPN/OpenVPN repository
56fc48e 
This avoids the error message triggering every night that the run
failed in forked repositories

Change-Id: Id95e0124d943912439c6ec6f562c0eb40d434163
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240506155831.3524-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28627.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 815df21)
@schwabe @cron2
Workaround issue in LibreSSL crashing when enumerating digests/ciphers
8aed156 
OpenBSD/LibreSSL reimplemented EVP_get_cipherbyname/EVP_get_digestbyname
and broke calling EVP_get_cipherbynid/EVP_get_digestbyname with an
invalid nid in the process so that it would segfault.

Workaround but doing that NULL check in OpenVPN instead of leaving it
to the library.

Github: see also libressl/openbsd#150

Change-Id: Ia08a9697d0ff41721fb0acf17ccb4cfa23cb3934
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20240508220540.12554-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28649.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit b3a271b)
@reynir @cron2
Only schedule_exit() once
65fb67c 
If an exit has already been scheduled we should not schedule it again.
Otherwise, the exit signal is never emitted if the peer reschedules the
exit before the timeout occurs.

schedule_exit() now only takes the context as argument. The signal is
hard coded to SIGTERM, and the interval is read directly from the
context options.

Furthermore, schedule_exit() now returns a bool signifying whether an
exit was scheduled; false if exit is already scheduled. The call sites
are updated accordingly. A notable difference is that management is only
notified *once* when an exit is scheduled - we no longer notify
management on redundant exit.

This patch was assigned a CVE number after already reviewed and ACKed,
because it was discovered that a misbehaving client can use the (now
fixed) server behaviour to avoid being disconnected by means of a
managment interface "client-kill" command - the security issue here is
"client can circumvent security policy set by management interface".

This only affects previously authenticated clients, and only management
client-kill, so normal renegotion / AUTH_FAIL ("your session ends") is not
affected.

CVE: 2024-28882

Change-Id: I9457f005f4ba970502e6b667d9dc4299a588d661
Signed-off-by: Reynir Björnsson <reynir@reynir.dk>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Message-Id: <20240516120434.23499-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28679.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 55bb326)
@fichtner @cron2
Allow to set ifmode for existing DCO interfaces in FreeBSD
2f2ff18 
While prexisting devices work well TUN/TAP the DCO interfaces require
setting the ifmode which cannot be done by FreeBSD base tooling.  In
peer-to-peer mode this is not a problem because that is the default mode.
Subnet mode, however, will fail to be set and the resulting connection does
not start:

  Failed to create interface ovpns2 (SIOCSIFNAME): File exists (errno=17)
  DCO device ovpns2 already exists, won't be destroyed at shutdown
  /sbin/ifconfig ovpns2 10.1.8.1/24 mtu 1500 up
  ifconfig: in_exec_nl(): Empty IFA_LOCAL/IFA_ADDRESS
  ifconfig: ioctl (SIOCAIFADDR): Invalid argument
  FreeBSD ifconfig failed: external program exited with error status: 1
  Exiting due to fatal error

Slightly restructure the code to catch the specific error
condition and execute dco_set_ifmode() in this case as well.

Signed-off-by: Franco Fichtner <franco@opnsense.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <AE20A784-506C-488B-9302-2D3AE775B168@opnsense.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28688.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 82036c1)
@flichtenheld @cron2
LZO: do not use lzoutils.h macros
1ae753e 
Instead of lzo_{free,malloc} we can just use the
free and malloc as the lzoutils.h header itself
suggests.

Change-Id: I32ee28fde5d38d736f753c782d88a81de7fe2980
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Message-Id: <20240604211708.32315-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28705.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit d601237)
@hwundram @cron2
Implement Windows CA template match for Crypto-API selector
dfbe11a 
The certificate selection process for the Crypto API certificates
is currently fixed to match on subject or identifier. Especially
if certificates that are used for OpenVPN are managed by a Windows CA,
it is appropriate to select the certificate to use by the template
that it is generated from, especially on domain-joined clients which
automatically acquire/renew the corresponding certificate.

The attached match implements the match on TMPL: with either a template
name (which is looked up through CryptFindOIDInfo) or by specifying the
OID of the template directly, which then is matched against the
corresponding X509 extensions specifying the template that the certificate
was generated from.

The logic requires to walk all certificates in the underlying store and
to match the certificate extensions directly. The hook which is
implemented in the certificate selection logic is generic to allow
other Crypto-API certificate matches to also be implemented at some
point in the future.

The logic to match the certificate template is taken from the
implementation in the .NET core runtime, see Pal.Windows/FindPal.cs in
in the implementation of System.Security.Cryptography.X509Certificates.

Change-Id: Ia2c3e4c5c83ecccce1618c43b489dbe811de5351
Signed-off-by: Heiko Wundram <heiko.wundram@gehrkens.it>
Signed-off-by: Hannes Domani <ssbssa@yahoo.de>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20240606103441.26598-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28726.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 13ee7f9)
@flichtenheld @cron2
Remove "experimental" denotation for --fast-io
d5c4c64 
This option is very old (from SVN days) and has been
used by Access Server for many years. I don't think it
makes sense to claim that it is "experimental" at this
point.

Change-Id: I913bb70c5e527e78e7cdb43110e23a8944f35a22
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Message-Id: <20240618120156.4836-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28772.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit f6ee77d)
@5andr0 @cron2
Implement server_poll_timeout for socks
94bfb71 
So far --server-poll-timeout was only applied
for HTTP proxies, apply it also to SOCKS proxies.

This removes the default 5 second socks connect timeout
which can be too small depending on network setup and
replaces it with the configurable overall connect timeout
(default 120 seconds).

Trac: #328
Github: fixes OpenVPN#267

Change-Id: I2b109f8c551c23045a1be355778b08f0fd4d309f
Signed-off-by: 5andr0 <sandro.trianni@gmail.com>
Tested-By: ValdikSS <valdikss@gmail.com>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240315162011.1661139-1-frank@lichtenheld.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28408.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit b3a68b8)
@schwabe @cron2
Properly handle null bytes and invalid characters in control messages
90e7a85 
This makes OpenVPN more picky in accepting control message in two aspects:
- Characters are checked in the whole buffer and not until the first
  NUL byte
- if the message contains invalid characters, we no longer continue
  evaluating a fixed up version of the message but rather stop
  processing it completely.

Previously it was possible to get invalid characters to end up in log
files or on a terminal.

This also prepares the logic a bit in the direction of having a proper
framing of control messages separated by null bytes instead of relying
on the TLS framing for that. All OpenVPN implementations write the 0
bytes between control commands.

This patch also include several improvement suggestion from Reynir
(thanks!).

CVE: 2024-5594

Reported-By: Reynir Björnsson <reynir@reynir.dk>
Change-Id: I0d926f910637dabc89bf5fa919dc6beef1eb46d9
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <a@unstable.cc>

Message-Id: <20240619103004.56460-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28791.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 414f428)
@lstipakov @cron2
interactive.c: Improve access control for gui<->service pipe
51301eb 
At the moment everyone but anonymous are permitted
to create a pipe with the same name as interactive service creates,
which makes it possible for malicious process with SeImpersonatePrivilege
impersonate as local user.

This hardens the security of the pipe, making it possible only for
processes running as SYSTEM (such as interactive service) create the
pipe with the same name.

While on it, replace EXPLICIT_ACCESS structures with SDDL string.

CVE: 2024-4877

Change-Id: I35e783b79a332d247606e05a39e41b4d35d39b5d
Reported by: Zeze with TeamT5 <zeze7w@gmail.com>
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20240619134451.222-1-lev@openvpn.net>
URL: https://www.mail-archive.com/search?l=mid&q=20240619134451.222-1-lev@openvpn.net
Signed-off-by: Gert Doering <gert@greenie.muc.de>
@cron2
preparing release 2.6.11
ddf6bf6 
version.m4, ChangeLog, Changes.rst

Signed-off-by: Gert Doering <gert@greenie.muc.de>
@itsGiaan @cron2
Http-proxy: fix bug preventing proxy credentials caching
ad0c2c0 
Caching proxy credentials was not working due to the
lack of handling already defined creds in get_user_pass(),
which prevented the caching from working properly.

Fix this issue by getting the value of c->first_time,
that indicates if we're at the first iteration
of the main loop and use it as second argument of the
get_user_pass_http(). Otherwise, on SIGUSR1 or SIGHUP
upon instance context restart credentials would be erased
every time.

The nocache member has been added to the struct
http_proxy_options and also a getter method to retrieve
that option from ssl has been added, by doing this
we're able to erase previous queried user credentials
to ensure correct operation.

Fixes: Trac #1187
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Change-Id: Ia3e06c0832c4ca0ab868c845279fb71c01a1a78a
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240623200551.20092-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28835.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 3cfd6f9)
@flichtenheld @cron2
configure: Try to detect LZO with pkg-config
3c43b01 
On most systems this should work just fine.

v2:
 - simplify code by removing -llzo special handling
v3:
 - reintroduce support for autodetection without pkg-config,
   no need to break backwards compatibility right now
v7:
 - Handle case correctly where lzo/lzo1x.h can not be included
   at all. On most distros this works even though the .pc
   file suggests to use it without. We had some partly
   solution for that but it wasn't really working.
v8:
 - Handle systems that do not implicitly include limits.h
   in configure test builds.
   lzodefs.h usually relies on lzoconf.h to include it.

Change-Id: I1c038dc4ec80d3499582d81eee61fee74f26e693
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20240626161921.179301-1-frank@lichtenheld.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28848.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 0ea5126)
@schwabe @cron2
Allow trailing \r and \n in control channel message
3435739 
Writing a reason from a script will easily end up adding extra \r\n characters
at the end of the reason. Our current code pushes this to the peer. So be more
liberal in accepting these message.

Github: closes OpenVPN#568

Change-Id: I47c992b6b73b1475cbff8a28f720cf50dc1fbe3e
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20240710140623.172829-1-frank@lichtenheld.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28910.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit be31325)
@cron2
preparing release 2.6.12
038a94b 
version.m4, ChangeLog, Changes.rst

Signed-off-by: Gert Doering <gert@greenie.muc.de>
@rishipal-Sophos
Merge tag 'v2.6.12' of https://github.com/OpenVPN/openvpn into bugfix…
595075a 
…/NCL-2203-SCC-Windows-Fix-vulnerability-in-OpenVPN

OpenVPN Release v2.6.12

2024.07.17 -- Version 2.6.12

Arne Schwabe (1):
      Allow trailing \r and \n in control channel message

Frank Lichtenheld (1):
      configure: Try to detect LZO with pkg-config

Gianmarco De Gregori (1):
      Http-proxy: fix bug preventing proxy credentials caching
maximmenshikov
maximmenshikov reviewed Jan 17, 2025
View reviewed changes
Copy link

@maximmenshikov maximmenshikov left a comment

Choose a reason for hiding this comment

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

The PR sounds ok, but I suggest to try openvpn-2.6.13 since it has already been released and since it has important Windows feature (such as credential protection) and more security fixes.

.github/workflows/build.yaml
include:
# macos14 and newer runners use ARM CPUs and homebrew uses /opt/homebrew/
Copy link

Choose a reason for hiding this comment

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

do you update MacOS build simultaneously? Could you tell the purpose?

Copy link

Choose a reason for hiding this comment

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

I checked original repo and it seems to be different there. Could you summarize your changes?

Copy link
Author

Choose a reason for hiding this comment

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

Currently, we are not using OpenVPN for macOS, but OpenVPN can be used for both platforms.
All build-related changes are managed by the OpenVPN team.
I have pulled all changes from the original repo for the tag-2.6.12.
Please review the code based on this tag you will found these changes in original repo as well ,
not the master branch or the release/2.6.0 branch.

Copy link

Choose a reason for hiding this comment

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

I think I looked at this particular tag, but OK. The most important is that it is not custom change.

@akodithy
Copy link

akodithy commented Jan 20, 2025 •
edited
Loading

As discussed, kindly check whether MultiThreaded option is already incorporated as part of sophos repo and if not, can we incorporate it similar to what Madan had done for NCL-1641.

Attached below is diffs file I had got for 2.5.6 sophos.
openvpnDiffs.txt

@rishipal-Sophos rishipal-Sophos merged commit faf2b92 into sophos_2.6.12 Jan 20, 2025
40 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximmenshikov maximmenshikov maximmenshikov left review comments

@akodithy akodithy akodithy approved these changes

@SeemaKodikanyana SeemaKodikanyana Awaiting requested review from SeemaKodikanyana

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.