NetBSD Problem Report #57919

From www@netbsd.org  Fri Feb  9 22:08:51 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 436FF1A9238
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  9 Feb 2024 22:08:51 +0000 (UTC)
Message-Id: <20240209220849.BF8EA1A9239@mollari.NetBSD.org>
Date: Fri,  9 Feb 2024 22:08:49 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: patching away -Werror in security/libfido2 broke it on NetBSD
X-Send-Pr-Version: www-1.0

>Number:         57919
>Category:       pkg
>Synopsis:       patching away -Werror in security/libfido2 broke it on NetBSD
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    pkg-manager
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Feb 09 22:10:00 +0000 2024
>Closed-Date:    Fri Mar 01 18:25:53 +0000 2024
>Last-Modified:  Fri Mar 01 18:25:53 +0000 2024
>Originator:     Taylor R Campbell
>Release:        pkgsrc-2023Q3, pkgsrc-2023Q4, pkgsrc-HEAD
>Organization:
The NetFIDO Foundation
>Environment:
LP64 BSD
>Description:
security/libfido2/patches/patch-CMakeLists.txt patches away all uses of -Werror, including this one:

+ try_compile(HAVE_POSIX_IOCTL
+     "${CMAKE_CURRENT_BINARY_DIR}/posix_ioctl_check.o"
+     "${CMAKE_CURRENT_SOURCE_DIR}/openbsd-compat/posix_ioctl_check.c"
+-    COMPILE_DEFINITIONS "-Werror -Woverflow -Wsign-conversion")
++    COMPILE_DEFINITIONS "-Woverflow -Wsign-conversion")

Unfortunately, libfido2 relies on this for correctness.  It tries to compile posix_ioctl_check.c with -Werror in order to detect whether ioctl takes int (as in POSIX) or unsigned long (as in BSD) for the command/request:

$ cat openbsd-compat/posix_ioctl_check.c
#include <sys/ioctl.h>

int
posix_ioctl_check(int fd)
{
	return ioctl(fd, -1, 0);
}

On a BSD system where ioctl takes unsigned long instead of int, the compiler might warn about passing -1 here -- and the libfido2 build relies on upgrading such a warning to an error to detect this.  Without that warning, it assumes ioctl takes int.

And on systems where it thinks ioctl takes int, libfido2 _explicitly casts_ all the input arguments to int before passing them.  Which causes sign-extension, on NetBSD, so that, for example, USB_HID_SET_RAW = 0x80046802 gets converted to (unsigned long)(int)USB_HID_SET_RAW = 0xffffffff80046802, which is not an ioctl that uhid(4) recognizes so it fails with EINVAL and libfido2 is completely broken in pkgsrc for interacting with fido2 devices.
>How-To-Repeat:
Plug in a USB FIDO device, so that it appears at uhid0; then run:

$ echo credential challenge | openssl sha256 -binary | base64 > cred_param
$ echo relying party >> cred_param
$ echo user name >> cred_param
$ dd if=/dev/urandom bs=1 count=32 | base64 >> cred_param
$ fido2-cred -M -i cred_param /dev/uhid0 | fido2-cred -V -o cred

Expected result: silent success, and ktrace shows:

kdump:
 13581  13581 fido2-cred CALL  ioctl(4,USB_HID_SET_RAW,0x7f7fff228c0c)
kdump -n:
 13581  13581 fido2-cred CALL  ioctl(4,0x80046802,0x7f7fff228c0c)

Actual result: fails with:
fido2-cred: fido_dev_open /dev/uhid0: FIDO_ERR_INTERNAL
fido2-cred: input error

And ktrace shows:

kdump:
 21174  21174 fido2-cred CALL  ioctl(4,_IOW('h',0x2,0x4),0x7f7fff1d8d1c)
kdump -n:
 21174  21174 fido2-cred CALL  ioctl(4,0xffffffff80046802,0x7f7fff1d8d1c)

(kdump(1) showing _IOW('h',0x2,0x4) for the sign-extended one is wrong; see https://gnats.NetBSD.org/57918 for that problem.
>Fix:
Yes, please!

- Maybe just unpatch out that -Werror.
- Maybe find a better way to do this detection.
- Maybe patch the NetBSD code to not use the IOCTL_REQ macro that conditionally casts to int.

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 10 Feb 2024 03:11:32 +0000
State-Changed-Why:
fix committed, needs pullup to 2023Q4


From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57919 CVS commit: pkgsrc/security/libfido2
Date: Sat, 10 Feb 2024 03:10:53 +0000

 Module Name:	pkgsrc
 Committed By:	riastradh
 Date:		Sat Feb 10 03:10:53 UTC 2024

 Modified Files:
 	pkgsrc/security/libfido2: Makefile distinfo
 	pkgsrc/security/libfido2/patches: patch-CMakeLists.txt

 Log Message:
 security/libfido2: Fix NetBSD build, PR pkg/57919.

 Patching away -Werror may be reasonable in general, but in this case
 it breaks libfido2's detection of whether ioctl takes int or unsigned
 long on NetBSD -- without -Werror, it wrongly concludes int, and
 proceeds to build a libfido2 that casts every ioctl command to int
 first, which leads to sign extension, which leads to the wrong ioctls
 being passed into the kernel, which leads libfido2 to fail in any
 attempts to open fido devices on NetBSD.


 To generate a diff of this commit:
 cvs rdiff -u -r1.13 -r1.14 pkgsrc/security/libfido2/Makefile
 cvs rdiff -u -r1.10 -r1.11 pkgsrc/security/libfido2/distinfo
 cvs rdiff -u -r1.1 -r1.2 \
     pkgsrc/security/libfido2/patches/patch-CMakeLists.txt

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 01 Mar 2024 15:12:42 +0000
State-Changed-Why:
pullup-pkgsrc #6838


From: "Benny Siegert" <bsiegert@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57919 CVS commit: [pkgsrc-2023Q4] pkgsrc/security/libfido2
Date: Fri, 1 Mar 2024 17:29:55 +0000

 Module Name:	pkgsrc
 Committed By:	bsiegert
 Date:		Fri Mar  1 17:29:55 UTC 2024

 Modified Files:
 	pkgsrc/security/libfido2 [pkgsrc-2023Q4]: Makefile distinfo
 	pkgsrc/security/libfido2/patches [pkgsrc-2023Q4]: patch-CMakeLists.txt

 Log Message:
 Pullup ticket #6838 - requested by riastradh
 security/libfido2: NetBSD build fix

 security/libfido2: Fix NetBSD support (PR 57919) by not patching
 away -Werror in a critical place where the build system relies on
 it to detect whether ioctl argument is int or long.  When the build
 system infers the wrong answer, it builds a libfido2 that doesn't
 work on NetBSD because it sends the wrong (sign-extended) ioctl
 commands to the kernel for USB stuff.

 Revisions pulled up:
 - security/libfido2/Makefile                                    1.14
 - security/libfido2/distinfo                                    1.11
 - security/libfido2/patches/patch-CMakeLists.txt                1.2

 ---
    Module Name:    pkgsrc
    Committed By:   riastradh
    Date:           Sat Feb 10 03:10:53 UTC 2024

    Modified Files:
            pkgsrc/security/libfido2: Makefile distinfo
            pkgsrc/security/libfido2/patches: patch-CMakeLists.txt

    Log Message:
    security/libfido2: Fix NetBSD build, PR pkg/57919.

    Patching away -Werror may be reasonable in general, but in this case
    it breaks libfido2's detection of whether ioctl takes int or unsigned
    long on NetBSD -- without -Werror, it wrongly concludes int, and
    proceeds to build a libfido2 that casts every ioctl command to int
    first, which leads to sign extension, which leads to the wrong ioctls
    being passed into the kernel, which leads libfido2 to fail in any
    attempts to open fido devices on NetBSD.


 To generate a diff of this commit:
 cvs rdiff -u -r1.13 -r1.13.2.1 pkgsrc/security/libfido2/Makefile
 cvs rdiff -u -r1.10 -r1.10.2.1 pkgsrc/security/libfido2/distinfo
 cvs rdiff -u -r1.1 -r1.1.4.1 \
     pkgsrc/security/libfido2/patches/patch-CMakeLists.txt

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: bsiegert@NetBSD.org
State-Changed-When: Fri, 01 Mar 2024 18:25:53 +0000
State-Changed-Why:
Pulled up


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2024 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.