Re: HID parser
On Sun, May 04, 2014 at 06:11:41PM +0200, Martin Pieuchot wrote: In December 2012 a user reported on misc@ that the Noppoo Mini Choc 84 USB keyboard does not work on OpenBSD [0]. More recently, mcbride@ and yasuoka@ contacted me because they have a mouse that is not properly recognized. Both issues are related to our HID descriptor parser. The chinese Rapoo V7 keyboard now works with this patch. At the time, I asked for something in: http://marc.info/?l=openbsd-techm=135941037714276w=2 The dmesg follows: OpenBSD 5.5-current (GENERIC) #4: Mon May 5 04:08:16 BRT 2014 r...@white.my.domain:/usr/src/sys/arch/amd64/compile/GENERIC real mem = 1687670784 (1609MB) avail mem = 1634091008 (1558MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xec710 (45 entries) bios0: vendor American Megatrends Inc. version X401U.313 date 05/14/2013 bios0: ASUSTeK COMPUTER INC. X401U acpi0 at bios0: rev 2 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP APIC FPDT MCFG ECDT HPET SSDT SSDT MSDM acpi0: wakeup devices SBAZ(S4) OHC1(S4) EHC1(S4) OHC2(S4) EHC2(S4) OHC3(S4) EHC3(S4) OHC4(S4) XHC0(S4) XHC1(S4) PE20(S4) PE21(S4) GLAN(S4) PE22(S4) PE23(S4) SLPB(S4) acpitimer0 at acpi0: 3579545 Hz, 32 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: AMD C-70 APU with Radeon(tm) HD Graphics, 998.31 MHz cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,MWAIT,SSSE3,CX16,POPCNT,NXE,MMXX,FFXSR,LONG,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,IBS,SKINIT,ITSC cpu0: 32KB 64b/line 2-way I-cache, 32KB 64b/line 8-way D-cache, 512KB 64b/line 16-way L2 cache cpu0: 8 4MB entries fully associative cpu0: DTLB 40 4KB entries fully associative, 8 4MB entries fully associative cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 199MHz cpu at mainbus0: not configured ioapic0 at mainbus0: apid 3 pa 0xfec0, version 21, 24 pins acpimcfg0 at acpi0 addr 0xe000, bus 0-255 acpiec0 at acpi0 acpihpet0 at acpi0: 14318180 Hz acpiprt0 at acpi0: bus 0 (PCI0) acpiprt1 at acpi0: bus 3 (PE20) acpiprt2 at acpi0: bus 6 (PE21) acpiprt3 at acpi0: bus -1 (PE22) acpiprt4 at acpi0: bus 7 (PE23) acpiprt5 at acpi0: bus -1 (BR15) acpiprt6 at acpi0: bus -1 (PCE7) acpiprt7 at acpi0: bus -1 (PCE8) acpiprt8 at acpi0: bus 1 (BR14) acpicpu0 at acpi0: C2, PSS acpitz0 at acpi0: critical temperature is 95 degC acpiac0 at acpi0: AC unit online acpibat0 at acpi0: BAT0 model X401-44 serial type LIon oem ASUSTek acpibtn0 at acpi0: PWRB acpibtn1 at acpi0: LID_ acpibtn2 at acpi0: SLPB acpivideo0 at acpi0: VGA_ cpu0: 998 MHz: speeds: 1000 800 MHz pci0 at mainbus0 bus 0 pchb0 at pci0 dev 0 function 0 AMD AMD64 14h Host rev 0x00 radeondrm0 at pci0 dev 1 function 0 ATI Radeon HD 7290 rev 0x00 drm0 at radeondrm0 radeondrm0: msi azalia0 at pci0 dev 1 function 1 ATI Radeon HD 6310 HD Audio rev 0x00: msi azalia0: no supported codecs ppb0 at pci0 dev 4 function 0 AMD AMD64 14h PCIE rev 0x00: msi pci1 at ppb0 bus 1 AMD Hudson-2 xHCI rev 0x03 at pci0 dev 16 function 0 not configured ahci0 at pci0 dev 17 function 0 AMD Hudson-2 SATA rev 0x40: msi, AHCI 1.3 scsibus1 at ahci0: 32 targets sd0 at scsibus1 targ 0 lun 0: ATA, Hitachi HTS54505, GG2O SCSI3 0/direct fixed naa.5000cca700c3196d sd0: 476940MB, 512 bytes/sector, 976773168 sectors ohci0 at pci0 dev 18 function 0 AMD Hudson-2 USB rev 0x11: apic 3 int 18, version 1.0, legacy support ehci0 at pci0 dev 18 function 2 AMD Hudson-2 USB2 rev 0x11: apic 3 int 17 usb0 at ehci0: USB revision 2.0 uhub0 at usb0 AMD EHCI root hub rev 2.00/1.00 addr 1 ohci1 at pci0 dev 19 function 0 AMD Hudson-2 USB rev 0x11: apic 3 int 18, version 1.0, legacy support ehci1 at pci0 dev 19 function 2 AMD Hudson-2 USB2 rev 0x11: apic 3 int 17 usb1 at ehci1: USB revision 2.0 uhub1 at usb1 AMD EHCI root hub rev 2.00/1.00 addr 1 piixpm0 at pci0 dev 20 function 0 AMD Hudson-2 SMBus rev 0x14: polling iic0 at piixpm0 piixpm0: exec: op 1, addr 0x18, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x19, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x1a, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x1b, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x1c, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x1d, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x1e, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x1f, cmdlen 1, len 0, flags 0x08: timeout, status 0xfBUSY,INTR,DEVERR,BUSERR piixpm0: exec: op 1, addr 0x20, cmdlen 1, len 0, flags 0x08: timeout, status
Re: malloc in libssl/src/apps
On Mon, 5 May 2014, Jean-Philippe Ouellet wrote:
On Mon, May 05, 2014 at 11:12:00AM +1000, Joel Sing wrote:
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
This one is a change in behaviour - if arg-count is 0 then previously
we zeroed arg-data; now we do not.
This one is calloc, not reallocarray, so unless I'm seriously missing
something obvious here, it is indeed zero'd, no?
Run the following before and after your change:
#include stdio.h
#include strings.h
#include openssl/bio.h
#include openssl/conf.h
#include apps.h
BIO *bio_err;
CONF *config;
int
main(int argc, char **argv)
{
char buf[128] = -one -two -three -four -five;
ARGS args;
int i;
memset(args, 0, sizeof(args));
chopup_args(args, buf, argc, argv);
for (i = 0; i args.count; i++)
printf(%i = %p\n, i, args.data[i]);
strlcpy(buf, -one -two, sizeof(buf));
chopup_args(args, buf, argc, argv);
for (i = 0; i args.count; i++)
printf(%i = %p\n, i, args.data[i]);
}
$ gcc -o chopup chopup.c /usr/src/lib/libssl/src/apps/apps.c -I
/usr/src/lib/libssl/src/apps -lcrypto
--
Action without study is fatal. Study without action is futile.
-- Mary Ritter Beard
[Patch fuse] fusefs_link return code
This patch fixes a bug where fusefs_link does not return an error on
subsequent invocations if a fuse filesystem does not implement hard
links.
As an aside, returning ENOSYS in this case is contrary to the link(2)
man page (and different again to the Open Group) but consistent with
the fuse implementation on Linux.
--
Helg xx...@msn.com
Index: fuse_vnops.c
===
RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
retrieving revision 1.16
diff -u -p -r1.16 fuse_vnops.c
--- fuse_vnops.c18 Mar 2014 08:51:53 - 1.16
+++ fuse_vnops.c4 May 2014 06:32:50 -
@@ -540,8 +540,10 @@ fusefs_link(void *v)
dip = VTOI(dvp);
fmp = (struct fusefs_mnt *)ip-ufs_ino.i_ump;
- if (!fmp-sess_init || (fmp-undef_op UNDEF_LINK))
+ if (!fmp-sess_init || (fmp-undef_op UNDEF_LINK)) {
+ error = ENOSYS;
goto out1;
+ }
fbuf = fb_setup(cnp-cn_namelen + 1, dip-ufs_ino.i_number,
FBT_LINK, p);
Re: There's no need for the link-layer address in the RB-tree...
On 25/04/14(Fri) 15:59, Martin Pieuchot wrote: On 25/04/14(Fri) 15:46, Martin Pieuchot wrote: [...] Regarding the tree, when Henning added ifa_add/del prior to its addition, he also used it for link-layer addresses. But ifa_ifwithaddr() was not dealing with link-layer addresses at that time! So when ifa_add/del got modified to add elements to the tree, all the lladdrs ended up in it. I'm talking bullshit here, it was already the first element of the list :) With r1.165 of net/route.c I believe that the tree is now ready for this. Somebody tried this diff? Does it break something in your setup? Ok?
Re: [Patch] Add router alert option to igmp packets
Hello Florian,
On 26/04/14(Sat) 22:22, Florian Riehm wrote:
On 04/26/14 20:35, Alexander Bluhm wrote:
/*
* To avoid byte-swapping the same value over and over again.
*/
FreeBSD has code matching this comment. In OpenBSD the code is
gone and so should the comment. Of course that is unrelated to
this diff.
I have removed the comment in my new diff also.
[...]
Be careful next time you send a diff, this one got reformatted and I
couldn't apply it correctly ;)
Here's an updated version of your diff that adds a check in case
m_get() returns NULL (very unlikely) and since I couldn't easily
understand the two fields set to 0x00, I decided to document them.
ok?
Index: igmp.c
===
RCS file: /home/ncvs/src/sys/netinet/igmp.c,v
retrieving revision 1.39
diff -u -p -r1.39 igmp.c
--- igmp.c 21 Apr 2014 12:22:26 - 1.39
+++ igmp.c 5 May 2014 13:32:12 -
@@ -103,6 +103,7 @@ int *igmpctl_vars[IGMPCTL_MAXID] = IGMPC
intigmp_timers_are_running;
static struct router_info *rti_head;
+static struct mbuf *router_alert;
struct igmpstat igmpstat;
void igmp_checktimer(struct ifnet *);
@@ -113,12 +114,34 @@ struct router_info * rti_find(struct ifn
void
igmp_init(void)
{
+ struct ipoption *ra;
- /*
-* To avoid byte-swapping the same value over and over again.
-*/
igmp_timers_are_running = 0;
rti_head = 0;
+
+ router_alert = m_get(M_DONTWAIT, MT_DATA);
+ if (router_alert == NULL) {
+ printf(%s: no mbuf\n, __func__);
+ return;
+ }
+
+ /*
+* Construct a Router Alert option (RAO) to use in report
+* messages as required by RFC2236. This option has the
+* following format:
+*
+* | 10010100 | 0100 | 2 octet value |
+*
+* where a value of 0 indicates that routers shall examine
+* the packet.
+*/
+ ra = mtod(router_alert, struct ipoption *);
+ ra-ipopt_dst.s_addr = INADDR_ANY;
+ ra-ipopt_list[0] = IPOPT_RA;
+ ra-ipopt_list[1] = 0x04;
+ ra-ipopt_list[2] = 0x00;
+ ra-ipopt_list[3] = 0x00;
+ router_alert-m_len = sizeof(ra-ipopt_dst) + ra-ipopt_list[1];
}
/* Return -1 for error. */
@@ -634,7 +657,7 @@ igmp_sendpkt(struct in_multi *inm, int t
imo.imo_multicast_loop = 0;
#endif /* MROUTING */
- ip_output(m, NULL, NULL, IP_MULTICASTOPTS, imo, NULL, 0);
+ ip_output(m, router_alert, NULL, IP_MULTICASTOPTS, imo, NULL, 0);
++igmpstat.igps_snd_reports;
}
Index: ip.h
===
RCS file: /home/ncvs/src/sys/netinet/ip.h,v
retrieving revision 1.14
diff -u -p -r1.14 ip.h
--- ip.h24 Oct 2013 15:21:21 - 1.14
+++ ip.h5 May 2014 13:32:12 -
@@ -150,6 +150,7 @@ struct ip {
#defineIPOPT_LSRR 131 /* loose source route */
#defineIPOPT_SATID 136 /* satnet id */
#defineIPOPT_SSRR 137 /* strict source route
*/
+#defineIPOPT_RA148 /* router alert */
/*
* Offsets to fields in options other than EOL and NOP.
not quite another erratum
A little background. Before we issue errata, we have to decide whether we should. That's usually pretty simple, but sometimes a bug looks exploitable when it isn't, or is exploitable when it looks benign. Clearly issuing zero errata isn't a workable solution, so we could issue errata for everything, but that leads to patch fatigue. Instead, we pick and choose as best we are able. Sometimes that's hard. Our primary focus is not on developing exploits. We have limited time and many other things to work on, and there is no reward for us to continue investigating exploit potential beyond a certain point. Referring now to a specific commit by jsing, see the commit for details. The bug's existence was publicly disclosed 3 years ago; I'm not about to reveal anything particularly secret. http://marc.info/?l=openbsd-cvsm=139913610412127w=2 The first issue is being able to trigger a memcpy with a small negative (huge unsigned) length. In general, any memory corruption can lead to code execution, but the specifics are application dependent. It's only a problem if there's privilege escalation involved. PEM encoded certs aren't used in the TLS wire protocol, for example, so network services are not affected. The second issue is that the incorrect handling of base64 padding will also decode to different bytes than a correct implementation. In general, any time two parsers process the same input and generate two different outputs that can lead to a security bypass, but again, the specifics are app dependent. We looked at these issues for a while and couldn't see a means to exploit them that would justify issuing a patch, but we can't rule out that possibility either. We neither want to cry wolf nor hide the truth. Lacking a clear consensus that we should patch or not patch, what you're getting instead is this email. (I initially proposed a patch and an email saying the patch was unnecessary, but it seems less confusing to simply send the email.) This email doesn't offer a lot of guidance about what to do, for which I apologize. At least now you know what we know.
wskbd(4) memory leak
We leave in a hotpluggable world. Let's free in detach() the memory allocated in attach(). While here I realize that M_TEMP might not be the best type to describe the keymap, M_DEVBUF maybe? Ok? Index: wskbd.c === RCS file: /cvs/src/sys/dev/wscons/wskbd.c,v retrieving revision 1.76 diff -u -p -r1.76 wskbd.c --- wskbd.c 26 Jan 2014 17:48:08 - 1.76 +++ wskbd.c 5 May 2014 08:11:32 - @@ -624,6 +624,8 @@ wskbd_detach(struct device *self, int f splx(s); } + free(sc-sc_map, M_TEMP); + /* locate the major number */ for (maj = 0; maj nchrdev; maj++) if (cdevsw[maj].d_open == wskbdopen)
Re: not quite another erratum
The process which came to the conclusion below took about 15-20 hours of accumulated developer time over the weekend. I'm almost running out of fingers counting developers. I wish we had the resources so that we could dedicate people to this in a more serious way. At least if we could dedicate them for a while, until this codebase has become wittled down. But we don't. Yes, that is a hint. It could be like the KMS effort over the last two years. Lacking that, this situation below is going to repeat itself again. Actually, it will repeat itself tomorrow A little background. Before we issue errata, we have to decide whether we should. That's usually pretty simple, but sometimes a bug looks exploitable when it isn't, or is exploitable when it looks benign. Clearly issuing zero errata isn't a workable solution, so we could issue errata for everything, but that leads to patch fatigue. Instead, we pick and choose as best we are able. Sometimes that's hard. Our primary focus is not on developing exploits. We have limited time and many other things to work on, and there is no reward for us to continue investigating exploit potential beyond a certain point. Referring now to a specific commit by jsing, see the commit for details. The bug's existence was publicly disclosed 3 years ago; I'm not about to reveal anything particularly secret. http://marc.info/?l=openbsd-cvsm=139913610412127w=2 The first issue is being able to trigger a memcpy with a small negative (huge unsigned) length. In general, any memory corruption can lead to code execution, but the specifics are application dependent. It's only a problem if there's privilege escalation involved. PEM encoded certs aren't used in the TLS wire protocol, for example, so network services are not affected. The second issue is that the incorrect handling of base64 padding will also decode to different bytes than a correct implementation. In general, any time two parsers process the same input and generate two different outputs that can lead to a security bypass, but again, the specifics are app dependent. We looked at these issues for a while and couldn't see a means to exploit them that would justify issuing a patch, but we can't rule out that possibility either. We neither want to cry wolf nor hide the truth. Lacking a clear consensus that we should patch or not patch, what you're getting instead is this email. (I initially proposed a patch and an email saying the patch was unnecessary, but it seems less confusing to simply send the email.) This email doesn't offer a lot of guidance about what to do, for which I apologize. At least now you know what we know.
uvm integer overflows
Inspired by some commits in bitrig, I did an audit for potential
integer overflows caused by converting a page number into an
offset/size/address by shifting by PAGE_SHIFT. While doing so, I
noticed that uvm_objwire/unwire really should really use voff_t
instead of off_t.
There is one potential overflow left that this diff doesn't address.
In uvm_swap.c:uvm_swap_io() there is a line that reads:
bp-b_dirtyend = npages PAGE_SHIFT;
Potentially this could overflow, but given that the b_dirtyend member
of struct buf is an int, there's not much we can do about this.
ok?
Index: uvm_aobj.c
===
RCS file: /cvs/src/sys/uvm/uvm_aobj.c,v
retrieving revision 1.63
diff -u -p -r1.63 uvm_aobj.c
--- uvm_aobj.c 30 Apr 2014 19:25:14 - 1.63
+++ uvm_aobj.c 5 May 2014 18:14:19 -
@@ -422,7 +422,8 @@ uao_shrink_flush(struct uvm_object *uobj
{
KASSERT(startpg endpg);
KASSERT(uobj-uo_refs == 1);
- uao_flush(uobj, startpg PAGE_SHIFT, endpg PAGE_SHIFT, PGO_FREE);
+ uao_flush(uobj, (voff_t)startpg PAGE_SHIFT,
+ (voff_t)endpg PAGE_SHIFT, PGO_FREE);
uao_dropswap_range(uobj, startpg, endpg);
}
@@ -909,14 +910,14 @@ uao_flush(struct uvm_object *uobj, voff_
if (flags PGO_ALLPAGES) {
start = 0;
- stop = aobj-u_pages PAGE_SHIFT;
+ stop = (voff_t)aobj-u_pages PAGE_SHIFT;
} else {
start = trunc_page(start);
stop = round_page(stop);
- if (stop (aobj-u_pages PAGE_SHIFT)) {
+ if (stop ((voff_t)aobj-u_pages PAGE_SHIFT)) {
printf(uao_flush: strange, got an out of range
flush (fixed)\n);
- stop = aobj-u_pages PAGE_SHIFT;
+ stop = (voff_t)aobj-u_pages PAGE_SHIFT;
}
}
@@ -1414,7 +1415,7 @@ uao_pagein_page(struct uvm_aobj *aobj, i
pg = NULL;
npages = 1;
- rv = uao_get(aobj-u_obj, pageidx PAGE_SHIFT,
+ rv = uao_get(aobj-u_obj, (voff_t)pageidx PAGE_SHIFT,
pg, npages, 0, VM_PROT_READ|VM_PROT_WRITE, 0, 0);
switch (rv) {
@@ -1511,7 +1512,7 @@ uao_dropswap_range(struct uvm_object *uo
int slot = elt-slots[j];
KASSERT(uvm_pagelookup(aobj-u_obj,
- (UAO_SWHASH_ELT_PAGEIDX_BASE(elt)
+
(voff_t)(UAO_SWHASH_ELT_PAGEIDX_BASE(elt)
+ j) PAGE_SHIFT) == NULL);
if (slot 0) {
Index: uvm_fault.c
===
RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
retrieving revision 1.72
diff -u -p -r1.72 uvm_fault.c
--- uvm_fault.c 13 Apr 2014 23:14:15 - 1.72
+++ uvm_fault.c 5 May 2014 18:14:19 -
@@ -622,7 +622,7 @@ ReFault:
/* wide fault (!narrow) */
nback = min(uvmadvice[ufi.entry-advice].nback,
(ufi.orig_rvaddr - ufi.entry-start) PAGE_SHIFT);
- startva = ufi.orig_rvaddr - (nback PAGE_SHIFT);
+ startva = ufi.orig_rvaddr - ((vsize_t)nback PAGE_SHIFT);
nforw = min(uvmadvice[ufi.entry-advice].nforw,
((ufi.entry-end - ufi.orig_rvaddr)
PAGE_SHIFT) - 1);
@@ -664,13 +664,13 @@ ReFault:
if (uobj) {
uoff = (startva - ufi.entry-start) + ufi.entry-offset;
(void) uobj-pgops-pgo_flush(uobj, uoff, uoff +
- (nback PAGE_SHIFT), PGO_DEACTIVATE);
+ ((vsize_t)nback PAGE_SHIFT), PGO_DEACTIVATE);
}
/* now forget about the backpages */
if (amap)
anons += nback;
- startva += (nback PAGE_SHIFT);
+ startva += ((vsize_t)nback PAGE_SHIFT);
npages -= nback;
centeridx = 0;
}
Index: uvm_object.c
===
RCS file: /cvs/src/sys/uvm/uvm_object.c,v
retrieving revision 1.7
diff -u -p -r1.7 uvm_object.c
--- uvm_object.c30 May 2013 15:17:59 - 1.7
+++ uvm_object.c5 May 2014 18:14:19 -
@@ -64,12 +64,12 @@ uvm_objinit(struct uvm_object *uobj, str
*/
int
-uvm_objwire(struct uvm_object *uobj, off_t start, off_t end,
+uvm_objwire(struct uvm_object *uobj, voff_t start, voff_t end,
struct pglist *pageq)
{
- int i, npages, error;
+ int i, npages, left, error;
struct vm_page *pgs[FETCH_PAGECOUNT];
- off_t offset = start, left;
+ voff_t offset = start;
left = (end - start)
Optional PIN for login_yubikey.c
I'd like to propose an addition to login_yubikey.c to support an
optional PIN.
I think that using the Yubikey for authentication is worthwhile. The
current implementation of login_yubikey.c, however, relies entirely on
the one-time password. I think the system would be stronger combining
the Yubikey with an additional PIN so that a compromise of the physical
security of the token doesn't compromise the associated account.
My work is loosely based off of Remi Locherer's suggested patch (link
below). Where it differs is that I'd like to add an optional additional
PIN to the authentication rather than use an existing credential, such
as the system password. My thinking is, if you are using the Yubikey
token already, the PIN probably can be a fairly low strength password.
The system password shouldn't be set to something simple. This allows
for relaxed rules the the Yubikey PIN without affecting the system
password policy as a whole.
http://comments.gmane.org/gmane.os.openbsd.tech/34693
I propose adding a new /var/db/yubi/$user.pin file that contains an
encrypted additional PIN (password). If present, this PIN must precede
the Yubikey one-time password when authenticating.
The password in $user.pin is encrypted in a manner similar to those in
/etc/master.passwd. Obviously, in a multi-user system some tool would
need to be devised to maintain these PIN/passwords. For my purposes,
the following works
---
# encrypt /var/db/yubi/$user.pin
password_goes_here
---
This has a couple nice side-effects:
1) By verifying hashed passwords I believe it is less susceptible to
timing attacks
2) Physical compromise of the Yubikey token does not immediately yield
access to the associated account
3) Compromise (read) of the contents of /var/db/yubi does not
immediately allow an attacker to access those accounts
This change is non-breaking in that, if the $user.pin file is not
present, login_yubikey works as before.
Thoughts?
Thanks,
Jeff
___
Common subdirectories: login_yubikey.orig/CVS and
login_yubikey.modified/CVS
diff -u login_yubikey.orig/login_yubikey.8
login_yubikey.modified/login_yubikey.8
--- login_yubikey.orig/login_yubikey.8 Mon May 5 13:57:11 2014
+++ login_yubikey.modified/login_yubikey.8 Mon May 5 14:00:45 2014
@@ -85,8 +85,10 @@
.Em user.uid ,
the user's key (32 hex digits) from
.Em user.key ,
-and the user's last-use counter from
-.Em user.ctr
+the user's last-use counter from
+.Em user.ctr ,
+and the user's PIN (optional) from
+.Em user.pin
in the
.Em /var/db/yubikey
directory.
@@ -99,6 +101,14 @@
does not have a last-use counter, a value of zero is used and
any counter is accepted during the first login.
.Pp
+If
+.Ar user
+does have a PIN file, the PIN must be provided before the one-time
password
+and the PIN will be verified (using
+.Xr crypt 8 ) against the contents of the PIN
+file. If the PIN file is not present, the user must provide only the
one-time
+password.
+.Pp
The one-time password provided by the user is decrypted using the
user's key.
After the decryption, the checksum embedded in the one-time password
@@ -124,4 +134,5 @@
.El
.Sh SEE ALSO
.Xr login 1 ,
-.Xr login.conf 5
+.Xr login.conf 5 ,
+.Xr crypt 8
diff -u login_yubikey.orig/login_yubikey.c
login_yubikey.modified/login_yubikey.c
--- login_yubikey.orig/login_yubikey.c Mon May 5 13:57:11 2014
+++ login_yubikey.modified/login_yubikey.c Mon May 5 13:57:23 2014
@@ -44,6 +44,7 @@
#include syslog.h
#include unistd.h
#include errno.h
+#include util.h
#include yubikey.h
@@ -54,15 +55,18 @@
#define AUTH_OK 0
#define AUTH_FAILED -1
+#define YUBIKEY_LENGTH 44
+
static const char *path = /var/db/yubikey;
static int clean_string(const char *);
static int yubikey_login(const char *, const char *);
+static int pin_login(const char *, const char *);
int
main(int argc, char *argv[])
{
- int ch, ret, mode = MODE_LOGIN;
+ int ch, ret, ret_pin, mode = MODE_LOGIN;
FILE *f = NULL;
char *username, *password = NULL;
char response[1024];
@@ -151,9 +155,33 @@
}
}
- ret = yubikey_login(username, password);
+ int password_length = strlen(password)-YUBIKEY_LENGTH;
+
+ /* if the password length 0 that means this isn't even long
enough to contain a valid yubi token */
+ if (password_length 0) {
+ syslog(LOG_INFO, user %s: reject, username);
+ fprintf(f, %s\n , BI_REJECT);
+ closelog();
+ return (EXIT_SUCCESS);
+ }
+
+ char password_pin[password_length +1];
+ char password_yubi[YUBIKEY_LENGTH + 1];
+
+ /* first password_length bytes are PIN */
+ strlcpy(password_pin, password, password_length + 1);
+
+ /* remaining 44 bytes are yubikey token */
+ strlcpy(password_yubi, (char*)password + password_length,
Re: malloc in libssl/src/apps
On Mon, May 05, 2014 at 07:31:34PM +1000, Joel Sing wrote: This one is calloc, not reallocarray, so unless I'm seriously missing something obvious here, it is indeed zero'd, no? Run the following before and after your change: Ah, yep. Can't believe I missed that (along with all the other obvious errors). I'm an idiot and this patch is full of shit, please ignore it completely. I'll go back to stuff I actually can do, and I'll be sure to read / test my changes while not half asleep before sending them in the future. Thanks to all for the feedback, and sorry for the noise.
Re: malloc in libssl/src/apps
On 05/06/14 00:10, Matthew Dempsky wrote:
On Sun, May 4, 2014 at 8:26 PM, Jean-Philippe Ouellet
jean-phili...@ouellet.biz wrote:
On Sun, May 04, 2014 at 11:30:40PM +0200, Alexander Hall wrote:
NULL theoretically could be != 0
Umm... short of something like:
#undef NULL
#define NULL I'm silly and want to break everything
or something, I don't see when that'd be the case.
I assumed from context that halex@ was asking about null pointers
having a non-zero memory representation, which is allowed by ISO C and
POSIX. But all OpenBSD platforms guarantee that all-bits-are-zero
pointer values are null pointers.
Yeah, I don't know much about standards, but that sounds like what my
mind was referring to. :-)
I'm almost certain that OpenSSH probably relies on this in places too,
so I think we're fine to rely on it in LibreSSL.
I believe a similar situation could appear with not explicitly
initialized global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
...
if ((ib = realloc(inodebuf, ibufsize)) == NULL)
...
}
Anyway, since people more knowledgeable than me on the topic (meaning
lots of people) do not consider this an issue, I guess it isn't.
/Alexander
Re: malloc in libssl/src/apps
On Mon, May 5, 2014 at 3:56 PM, Alexander Hall alexan...@beard.se wrote:
I believe a similar situation could appear with not explicitly initialized
global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
This is safe too: C requires that pointer variables with static
storage duration like this be initialized to a null pointer (C99
section 6.7.8 paragraph 10), not to a sequence of 0 bytes.
Questions about C (was: Re: malloc in libssl/src/apps)
Matthew Dempsky matt...@dempsky.org writes:
On Mon, May 5, 2014 at 3:56 PM, Alexander Hall alexan...@beard.se wrote:
I believe a similar situation could appear with not explicitly initialized
global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
This is safe too: C requires that pointer variables with static
storage duration like this be initialized to a null pointer (C99
section 6.7.8 paragraph 10), not to a sequence of 0 bytes.
People often ask where to find this kind of information, but the
C99 standard[1] is actually available free of charge, same for C11[2].
I also like to point them at the C FAQ[3].
[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
[2] http://www.open-std.org/jtc1/sc22/wg14/www/standards.html
[3] http://c-faq.com/
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: malloc in libssl/src/apps
On May 6, 2014 1:34:01 AM CEST, Matthew Dempsky matt...@dempsky.org wrote:
On Mon, May 5, 2014 at 3:56 PM, Alexander Hall alexan...@beard.se
wrote:
I believe a similar situation could appear with not explicitly
initialized
global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
This is safe too: C requires that pointer variables with static
storage duration like this be initialized to a null pointer (C99
section 6.7.8 paragraph 10), not to a sequence of 0 bytes.
Cool, thanks.
