On 2019/06/03 21:38, Rafael Sadowski wrote:
> On Sun Jun 02, 2019 at 05:41:37PM +0200, Jeremie Courreges-Anglas wrote:
> > On Sun, Jun 02 2019, Rafael Sadowski <[email protected]> wrote:
> > > Update keepassxc to 2.4.2
> > >
> > > CHANGELOG:
> > > https://github.com/keepassxreboot/keepassxc/releases/tag/2.4.2
> > >
> > > I send the Alloc class patch upsteream as usual. Tests and feedback
> > > welcome.
> >
> > I didn't check anything else, but the Alloc class patch shouldn't be
> > sent upstream as-is, IMO. Please see below.
> >
> > > RS
> > >
> > > ===================================================================
> > > RCS file: patches/patch-src_core_Alloc_cpp
> > > diff -N patches/patch-src_core_Alloc_cpp
> > > --- /dev/null 1 Jan 1970 00:00:00 -0000
> > > +++ patches/patch-src_core_Alloc_cpp 2 Jun 2019 09:44:06 -0000
> > > @@ -0,0 +1,23 @@
> > > +$OpenBSD$
> > > +
> > > +Index: src/core/Alloc.cpp
> > > +--- src/core/Alloc.cpp.orig
> > > ++++ src/core/Alloc.cpp
> > > +@@ -20,6 +20,8 @@
> > > + #include <sodium.h>
> > > + #ifdef Q_OS_MACOS
> > > + #include <malloc/malloc.h>
> > > ++#elif defined(Q_OS_OPENBSD)
> > > ++#include <stdlib.h>
> > > + #else
> > > + #include <malloc.h>
> > > + #endif
> > > +@@ -61,7 +63,7 @@ void operator delete(void* ptr) noexcept
> > > + ::operator delete(ptr, _msize(ptr));
> > > + #elif defined(Q_OS_MACOS)
> > > + ::operator delete(ptr, malloc_size(ptr));
> > > +-#elif defined(Q_OS_UNIX)
> > > ++#elif defined(Q_OS_LINUX)
> > > + ::operator delete(ptr, malloc_usable_size(ptr));
> > > + #else
> > > + // whatever OS this is, give up and simply free stuff
> > >
> >
> > I think the port's cmake setup should check for malloc_usable_size and
> > malloc.h, use them if available, and fall back on stdlib.h / std::free()
> > if not. OpenBSD is not the only OS where malloc.h isn't available, and
> > Linux (glibc really) isn't the only OS where malloc_usable_size() is
> > available. This kind of #ifdef practice is actively harmful for
> > portability.
> >
> > Also if the goal is to securely erase memory before freeing it,
> > maybe freezero(3) is a possible alternative?
> >
> > --
> > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
> >
>
> Thanks Jeremie for saving me from a mistake.
>
> There is no way to fix this on OpenBSD so let's disable for now. New
> diff which disabled the custom delete operator.
>
> General question, what does it make sense to delete each heap allocated
> (just allocated memory with "new") memory so?
AIUI they aren't tracking every allocation which _does_ use memory to hold
some sensitive data, so are blindly zeroing everything they can when the
allocation is deleted. Might slow things down a bit but this shouldn't be
a performance critical program anyway and why not wipe the things where
they can.
> In my opinion, we're not losing anything here.
>
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/security/keepassxc/Makefile,v
> retrieving revision 1.20
> diff -u -p -u -p -r1.20 Makefile
> --- Makefile 21 Apr 2019 11:29:44 -0000 1.20
> +++ Makefile 3 Jun 2019 19:17:16 -0000
> @@ -2,7 +2,7 @@
>
> COMMENT = management tool for password and sensitive data
>
> -V = 2.4.1
> +V = 2.4.2
> DISTNAME = keepassxc-${V}
>
> CATEGORIES = security
> @@ -16,7 +16,7 @@ PERMIT_PACKAGE_CDROM = Yes
>
> WANTLIB += ${COMPILER_LIBCXX} Qt5Concurrent Qt5Core Qt5DBus Qt5Gui
> WANTLIB += Qt5Network Qt5Svg Qt5Widgets Qt5X11Extras X11 Xi Xtst
> -WANTLIB += argon2 c gcrypt gpg-error m qrencode z
> +WANTLIB += argon2 c gcrypt gpg-error m qrencode sodium z
>
> MASTER_SITES =
> https://github.com/keepassxreboot/keepassxc/releases/download/${V}/
> EXTRACT_SUFX = -src.tar.xz
> @@ -25,6 +25,7 @@ MODULES = x11/qt5 \
> devel/cmake
>
> LIB_DEPENDS = security/libgcrypt \
> + security/libsodium \
> security/argon2 \
> graphics/libqrencode \
> x11/qt5/qtsvg \
> @@ -36,8 +37,8 @@ RUN_DEPENDS = devel/desktop-file-utils \
>
> CONFIGURE_ARGS= -DCMAKE_INSTALL_MANDIR="man" \
> -DWITH_GUI_TESTS=ON \
> - -DWITH_XC_SSHAGENT=ON \
> - -DWITH_XC_AUTOTYPE=ON
> + -DWITH_XC_AUTOTYPE=ON \
> + -DWITH_XC_SSHAGENT=ON
>
> TEST_IS_INTERACTIVE = X11
>
> @@ -52,10 +53,8 @@ WANTLIB += yubikey ykpers-1
> .endif
>
> .if ${FLAVOR:Mbrowser}
> -LIB_DEPENDS += security/libsodium
> CONFIGURE_ARGS += -DWITH_XC_BROWSER=ON \
> -DWITH_XC_NETWORKING=ON
> -WANTLIB += sodium
> .endif
>
> post-patch:
> Index: distinfo
> ===================================================================
> RCS file: /cvs/ports/security/keepassxc/distinfo,v
> retrieving revision 1.11
> diff -u -p -u -p -r1.11 distinfo
> --- distinfo 21 Apr 2019 11:29:44 -0000 1.11
> +++ distinfo 3 Jun 2019 19:17:16 -0000
> @@ -1,2 +1,2 @@
> -SHA256 (keepassxc-2.4.1-src.tar.xz) =
> Dal70SedG5sGpjufcjtDq4wHi38SA9bBNQT91HNUias=
> -SIZE (keepassxc-2.4.1-src.tar.xz) = 3277856
> +SHA256 (keepassxc-2.4.2-src.tar.xz) =
> FfptCikgVYZLGXnGcfE+W65QVuGeKAwwzBzwuW6lYTg=
> +SIZE (keepassxc-2.4.2-src.tar.xz) = 3290468
> Index: patches/patch-src_CMakeLists_txt
> ===================================================================
> RCS file: patches/patch-src_CMakeLists_txt
> diff -N patches/patch-src_CMakeLists_txt
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_CMakeLists_txt 3 Jun 2019 19:17:16 -0000
> @@ -0,0 +1,19 @@
> +$OpenBSD$
> +
> +https://github.com/keepassxreboot/keepassxc/pull/3020
Including this in the comment is a bit confusing because normally when
we include a URL to a pull request or commit, we are telling the reader
where the patch came from.
> +Remove the custom delete operator which zeroes out allocated memory before
> +freeing it. OpenBSD does not and will implement glibc's malloc_usable_size(3)
> +interface. "The main use of this function is for debugging and
> introspection."
>
> +Index: src/CMakeLists.txt
> +--- src/CMakeLists.txt.orig
> ++++ src/CMakeLists.txt
> +@@ -24,7 +24,6 @@ if(NOT ZXCVBN_LIBRARIES)
> + endif(NOT ZXCVBN_LIBRARIES)
> +
> + set(keepassx_SOURCES
> +- core/Alloc.cpp
> + core/AutoTypeAssociations.cpp
> + core/AutoTypeMatch.cpp
> + core/Compare.cpp
Rather than killing the whole file I think you should patch Alloc.cpp instead.
31 /**
32 * Custom sized delete operator which securely zeroes out allocated
33 * memory before freeing it (requires C++14 sized deallocation support).
34 */
35 void operator delete(void* ptr, std::size_t size) noexcept
36 {
37 if (!ptr) {
38 return;
39 }
40
41 sodium_memzero(ptr, size);
42 std::free(ptr);
43 }
44
45 void operator delete[](void* ptr, std::size_t size) noexcept
46 {
47 ::operator delete(ptr, size);
48 }
These first two replace the C++14 sized "delete" / "delete[]" so in cases
where those are used it already knows how much to zero, they should work
on OpenBSD too.
50 /**
51 * Custom delete operator which securely zeroes out
52 * allocated memory before freeing it.
53 */
54 void operator delete(void* ptr) noexcept
55 {
56 if (!ptr) {
57 return;
58 }
59
60 #if defined(Q_OS_WIN)
61 ::operator delete(ptr, _msize(ptr));
62 #elif defined(Q_OS_MACOS)
63 ::operator delete(ptr, malloc_size(ptr));
64 #elif defined(Q_OS_UNIX)
65 ::operator delete(ptr, malloc_usable_size(ptr));
66 #else
67 // whatever OS this is, give up and simply free stuff
68 std::free(ptr);
69 #endif
70 }
71
72 void operator delete[](void* ptr) noexcept
73 {
74 ::operator delete(ptr);
75 }
And these two are for the traditional non-sized "delete" / "delete[]"
where it's relying on the various nonportable functions to try to figure it
out, it's these that I think there is nothing we can do to sanely help.
