First off all thanks jca@ and sthen@ for the energy you put in this topic. On Mon Jun 03, 2019 at 09:39:16PM +0100, Stuart Henderson wrote: > 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 <raf...@sizeofvoid.org> 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.
Valid point what do you think about: Revert a part from: https://github.com/keepassxreboot/keepassxc/pull/3020 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." From http://man7.org/linux/man-pages/man3/malloc_usable_size.3.html > > > +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. That was my first idea. But ... > > 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. > They don't use C++14 delete operators. That's why it doesn't make sense to me. So it would just be an illusion > 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. > The only way I see it is to look at each case individually. IMHO they did it right with FileKey.cpp, PasswordKey.cpp etc. but not with Alloc.cpp.
