relayd - HTTP Desync Attacks
Hi guys! I've been reading about HTTP Desync Attacks lately so I took a
look to relayd's source code to check if it's likely to be exploited.
I wasn't able to do a POC (I don't have a OBSD installation at the moment)
but I think it is.
I'm going to install a OpenBSD as soon as I can in order to test the
current code and send a patch if needed but I want to tell you what I've
seen so far.
There I go:
If a malicious user split a "Transfer-Encoding: chunked" http header in
multiple lines along with a Content-Length header it would trick relayd to
use the content length header while forwarding the
Transfer-Encoding header downstream.
The problem is in relay_read_http:
if (strcasecmp("Transfer-Encoding", key) == 0 &&
strcasecmp("chunked", value) == 0)
desc->http_chunked = 1;
The check for key == "Transfer-Encoding" && value == chunked isn't deferred
until the full header is read and it should.
Maybe we could defer the check until we are out of the loop.
Replacing
if (desc->http_chunked) {
/* Chunked transfer encoding */
cre->toread = TOREAD_HTTP_CHUNK_LENGTH;
bev->readcb = relay_read_httpchunks;
}
by doing a lookup into desc->http_headers
In order to achieve this, the function kv_extend should be modified in
order to alter the structure pointed by the first parameter (today it does
nothing with the first parameter)
Also, It seems like the "lookup" label along with the goto lookup sentence
could be removed.
Payload
POST ...
...
Transfer-Encoding:
chunked
...
Content-Length: whatever greater than the chunk
10
1234567890123456
POISON!
If I'm right this payload would result in relayd honouring the
Content-Length header (breaking RFC 2616) and probably poisoning the socket
(assuming that the downstream server would honour the Transfer-Encoding
header)
PD: I haven't digged enough to realize if relayd reuse downstream
connections among different clients so I can't say the real impact of this
issue.
Best regards
Pablo
Use `if (retval == -1)' instead of 'if (retval < 0)'
Hi tech, Use `if (retval == -1)' instead of 'if (retval < 0)' when check the return value of system call. How about it? RCS file: /cvs/src/lib/libedit/readline.c,v retrieving revision 1.28 diff -u -p -u -r1.28 readline.c --- readline.c 28 Jun 2019 13:32:42 - 1.28 +++ readline.c 14 Aug 2019 04:38:55 - @@ -2112,7 +2112,7 @@ _rl_event_read_char(EditLine *el, wchar_ return -1; #endif - if (num_read < 0 && errno == EAGAIN) + if (num_read == -1 && errno == EAGAIN) continue; if (num_read == 0) continue; -- ASOU Masato
pms(4): elantech-v4 detection
With this patch, pms recognizes all elantech-v4 touchpads (see
https://marc.info/?l=openbsd-tech=156554256223597=2 ). Some
models may have external hardware buttons, they are identified by
checking a flag in the firmware version number.
OK?
Index: dev/pckbc/pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.88
diff -u -p -r1.88 pms.c
--- dev/pckbc/pms.c 26 Jan 2019 11:57:21 - 1.88
+++ dev/pckbc/pms.c 13 Aug 2019 21:46:49 -
@@ -143,6 +143,7 @@ struct elantech_softc {
int max_x, max_y;
int old_x, old_y;
};
+#define ELANTECH_IS_CLICKPAD(sc) (((sc)->elantech->fw_version & 0x1000) != 0)
struct pms_softc { /* driver status information */
struct device sc_dev;
@@ -1945,9 +1946,7 @@ elantech_get_hwinfo_v4(struct pms_softc
if (synaptics_query(sc, ELANTECH_QUE_FW_VER, _version))
return (-1);
- if ((fw_version & 0x0f) >> 16 != 6
- && (fw_version & 0x0f) >> 16 != 8
- && (fw_version & 0x0f) >> 16 != 15)
+ if ((fw_version & 0x0f) >> 16 < 6)
return (-1);
elantech->fw_version = fw_version;
@@ -1975,7 +1974,8 @@ elantech_get_hwinfo_v4(struct pms_softc
elantech->flags |= ELANTECH_F_TRACKPOINT;
hw->type = WSMOUSE_TYPE_ELANTECH;
- hw->hw_type = WSMOUSEHW_CLICKPAD;
+ hw->hw_type = (ELANTECH_IS_CLICKPAD(sc)
+ ? WSMOUSEHW_CLICKPAD : WSMOUSEHW_TOUCHPAD);
hw->mt_slots = ELANTECH_MAX_FINGERS;
elantech->width = hw->x_max / (capabilities[1] - 1);
@@ -2179,8 +2179,9 @@ pms_enable_elantech_v4(struct pms_softc
goto err;
}
- printf("%s: Elantech Clickpad, version %d, firmware 0x%x\n",
- DEVNAME(sc), 4, sc->elantech->fw_version);
+ printf("%s: Elantech %s, version 4, firmware 0x%x\n",
+ DEVNAME(sc), (ELANTECH_IS_CLICKPAD(sc) ? "Clickpad"
+ : "Touchpad"), sc->elantech->fw_version);
if (sc->elantech->flags & ELANTECH_F_TRACKPOINT) {
a.accessops = _sec_accessops;
Re: installer: clean up clean_old()
Theo Buehler wrote: > On Tue, Aug 13, 2019 at 09:59:28PM +0200, Christian Weisgerber wrote: > > * Remove syspatch files from the installed system and not the ramdisk. > > * Use extended globs and generally adopt to the style of this script. > > > > ok? > > I'm ok with your patch. One suggestion: > > > if [[ -f /mnt/usr/bin/clang ]]; then > > Could you add '&& -d /mnt/usr/lib/clang' to check whether the directory > exists before trying to cd into it (similarly for /mnt/usr/bin/gcc and > gcc-libs)? Then might as well just -d only, and skip the -f But then people who mangle their system with symbolic links will lose Perhaps that is actually for the best
Re: installer: clean up clean_old()
On Tue, Aug 13, 2019 at 09:59:28PM +0200, Christian Weisgerber wrote:
> * Remove syspatch files from the installed system and not the ramdisk.
> * Use extended globs and generally adopt to the style of this script.
>
> ok?
I'm ok with your patch. One suggestion:
> if [[ -f /mnt/usr/bin/clang ]]; then
Could you add '&& -d /mnt/usr/lib/clang' to check whether the directory
exists before trying to cd into it (similarly for /mnt/usr/bin/gcc and
gcc-libs)?
On amd64, /usr/bin/clang is in the base set due to KARL while the
/usr/lib/clang directory is shipped as part of the comp set. I know we
don't want to support installs that skip some sets, but still.
> - CVER=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q)
> - rm -rf -- `ls -d /mnt/usr/lib/clang/* | grep -v "${CVER}$"`
> + _cver=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q) &&
> + rm -rf /mnt/usr/lib/clang/!($_cver)
> fi
Re: iwm(4): fix ccmp decrypt edge cases
Hi, (cc'ed to bugs@ as well)
On Tue, Aug 13, 2019 at 02:58:05PM +0200, Stefan Sperling wrote:
> On Tue, Aug 13, 2019 at 09:40:22AM -0300, Martin Pieuchot wrote:
> >
> > How does the stack crashes?
>
> Jesper only sent me a screen shot and no public bug report :(
>
I've had a really busy week and you asked me to transcribe the
screenshot, which took a extra time since I couldn't use OCR. I've also
tried to reproduce it and running solely on wireless the last couple of
days, but sadly no luck. I ran chrome, rtorrent and was starting the
tor-browser when the crash happened, but I doubt that really matters.
I now noticed the trace was in /var/log/messages, *after* I copied it
manually. Oh well... :-)
I also apologize for the noise and the column length of this mail, I
just want to provide as much as possible and copied it as-is. The stack
trace below is written by hand, so typos might occur. Doublecheck the
screenshot (see below).
--8<--
restore_fbdev_mode_atomic(80862e00,1) at restore_fbdev-mode_atomic+0x1ac
drm_fb_helper_restore_fbdev_mode_unlocked(80862e00) at
drm_fb_helper_restore_fbdev_mode_unlocked+0x76
intel_fbdev_restore_mode(80147078) at intel_fbdev_restore_mode+0x33
db_ktrap(6,0,80002227b4a0) at db_ktrap+0x2c
kerntrap(80002227b4a0) at kerntrap+0xa2
alltraps_kern_meltdown(6,0,80002226b760,0,0,1) at
alltraps_kern_meltdown+0x7b
AES_Encrypt_ECB(0,80002227b760,80002227b760,1) at AES_Encrypt_ECB+0xd6
ieee80211_ccmp_phase1(0,fd800239700c,53,5e4,80002227b760,80002227b700)
at ieee80211_ccmp_phase1+0x1fc
ieee80211_ccmp_decrypt(80183048,fd808ce4ee00,80dba1a0) at
ieee80211_ccmp_decrypt+0x238
ieee80211_input(80183048,fd808ce4ee00,80dba000,81368038)
at ieee80211_input+0xa1f
ieee80211_ba_move_window(80183048,80dba000,0,9d) at
ieee80211_ba_move_window+0xb7
ieee80211_input(80183048,fd8095616800,80dba000,80002227baa0)
at ieee80211_input+0x71d
iwm_rx_rx_mpdu(80183000,fd800238f000,801e3d00) at
iwm_rx_rx_mpdu+0x421
iwm_notif_intr(80183000) at iwm_notif_intr+0x67b
iwm_intr(80183000) at iwm_intr+0x389
intr_handler(80002227bc50,80143180) at intr_handler+0x6e
Xintr_ioapic_edge22_untramp(0,0,1388,0,0,81d3c6f8) at
Xintr_ioapic_edge22_untramp+0x19f
acpicpu_idle() at acpicpu_idle+0x1d2
sched_idle(81d3bff0) at sched_idle+0x225
end trace frame: 0x0, count: 231
End of stack trace.
splassert: pool_do_put: want 0 have 7
Starting stack trace...
pool_do_put(81de50d0,fd81f6054f20) at pool_do_put+0x40
pool_put(81de50d0,fd81f6054f20) at pool_put+0x5a
futex_wait(3e157caf160,c8,3e168a70960,2) at futex_wait+0x1fe
sys_futex(800044028628,800044767590,8000447675f0) at sys_futex+0x94
syscall(800044767660) at syscall+0x389
Xsyscall(0,53,16,53,3,3e1b03d0400) at Xsyscall+0x128
end of kernel
end trace frame: 0x3e168a709d0, count: 251
End of stack trace.
splassert: assertwaitok: want 0 have 7
Starting stack trace...
assertwaitok() at assertwaitok+0x40
mi_switch() at mi_switch+0x40
preempt() at preempt+0x5c
ast(800044767660) at ast+0xb3
Xsyscall(0,3c,16,53,3,3e1b03d0400) at Xsyscall+0x156
end of kernel
end trace frame: 0x3e168a709d0, count: 252
End of stack trace.
kernel: page fault trap, code=0
Stopped at AES_Encrypt_ECB+0xd6:movl0x2d0(%rax),%edi
ddb{0}>
--8<--
The above mentioned screenshots can also be found at:
https://ifconfig.se/IMG_5204.jpg
https://ifconfig.se/IMG_5205.jpg
https://ifconfig.se/IMG_5206.jpg
https://ifconfig.se/IMG_5207.jpg
https://ifconfig.se/IMG_5208.jpg
https://ifconfig.se/IMG_5209.jpg
https://ifconfig.se/IMG_5210.jpg
https://ifconfig.se/IMG_5211.jpg
https://ifconfig.se/IMG_5212.jpg
https://ifconfig.se/IMG_5213.jpg
https://ifconfig.se/IMG_5214.jpg
https://ifconfig.se/IMG_5215.jpg
https://ifconfig.se/IMG_5216.jpg
I sadly don't have a dmesg from the day, but tried to extract as much as
I could from /var/log/messages:
--8<--
OpenBSD 6.5-current (GENERIC.MP) #186: Thu Aug 8 21:35:57 MDT 2019
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 8261529600 (7878MB)
avail mem = 8000983040 (7630MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xacbfd000 (65 entries)
bios0: vendor LENOVO version "N14ET50W (1.28 )" date 03/21/2019
bios0: LENOVO 20BS00A5MS
acpi0 at bios0: ACPI 5.0
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SLIC ASF! HPET ECDT APIC MCFG SSDT SSDT SSDT SSDT SSDT
SSDT SSDT SSDT SSDT PCCT SSDT UEFI MSDM BATB FPDT UEFI DMAR
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP2(S4) XHCI(S3) EHC1(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpihpet0 at acpi0: 14318179 Hz
acpiec0 at acpi0
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-5500U CPU @ 2.40GHz, 2295.13 MHz,
installer: clean up clean_old()
* Remove syspatch files from the installed system and not the ramdisk.
* Use extended globs and generally adopt to the style of this script.
ok?
I'm not very happy with the way the clang version is determined.
If we ever were to move to 10.0.0, this would remove the wrong
directory. I considered examining the output of "chroot /mnt
/usr/bin/clang -v", but I don't like that much either.
Index: distrib/miniroot/install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1138
diff -u -p -r1.1138 install.sub
--- distrib/miniroot/install.sub13 Aug 2019 07:47:40 - 1.1138
+++ distrib/miniroot/install.sub13 Aug 2019 19:45:23 -
@@ -2860,14 +2860,16 @@ __EOT
}
clean_old() {
+ local _cver
+
if [[ -f /mnt/usr/bin/gcc ]]; then
- rm -rf -- `ls -d /mnt/usr/lib/gcc-lib/* | grep -v ${VNAME}$`
+ rm -rf /mnt/usr/lib/gcc-lib/!(*$VNAME)
fi
if [[ -f /mnt/usr/bin/clang ]]; then
- CVER=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q)
- rm -rf -- `ls -d /mnt/usr/lib/clang/* | grep -v "${CVER}$"`
+ _cver=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q) &&
+ rm -rf /mnt/usr/lib/clang/!($_cver)
fi
- rm -rf /var/syspatch/*
+ rm -rf /mnt/var/syspatch/*
}
do_autoinstall() {
--
Christian "naddy" Weisgerber na...@mips.inka.de
snmp(1): Better error reporting on malformed packets
Right now if we receive a malformed reply (apart from potentially
crashing[0]) we return a rather unsightly and uninformative error
message:
$ LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1 snmp getnext -v2c
-cpublic 127.0.0.1 ifInDiscards.0
snmp: getnext: Undefined error: 0
This diff always sets errno to EPROTO if we can't parse the packet.
$ LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1 ./obj/snmp getnext -v2c
-cpublic 127.0.0.1 ifInDiscards.0
snmp: getnext: Protocol error
(note that I malformed snmpd to return "{o}" for this case).
This diff also retries the packet if a malformed reply is received.
This is similar to what netsnmp does.
OK?
martijn@
[0] https://marc.info/?l=openbsd-tech=156570856731073=2
Index: snmp.c
===
RCS file: /cvs/src/usr.bin/snmp/snmp.c,v
retrieving revision 1.1
diff -u -p -r1.1 snmp.c
--- snmp.c 9 Aug 2019 06:17:59 - 1.1
+++ snmp.c 13 Aug 2019 17:25:28 -
@@ -254,26 +254,51 @@ snmp_resolve(struct snmp_agent *agent, s
if (ret <= 0)
goto fail;
ber_set_readbuf(, buf, ret);
- if ((message = ber_read_elements(, NULL)) == NULL)
- goto fail;
+ if ((message = ber_read_elements(, NULL)) == NULL) {
+ direction = POLLOUT;
+ tries--;
+ continue;
+ }
if (ber_scanf_elements(message, "{ise", , ,
- ) != 0)
- goto fail;
+ ) != 0) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
+ continue;
+ }
/* Skip invalid packets; should not happen */
if (version != agent->version ||
- strcmp(community, agent->community) != 0)
+ strcmp(community, agent->community) != 0) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
continue;
+ }
/* Validate pdu format and check request id */
if (ber_scanf_elements(pdu, "{iSSe", , ) != 0 ||
- varbind->be_encoding != BER_TYPE_SEQUENCE)
- goto fail;
- if (rreqid != reqid)
+ varbind->be_encoding != BER_TYPE_SEQUENCE) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
continue;
+ }
+ if (rreqid != reqid) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
+ continue;
+ }
for (varbind = varbind->be_sub; varbind != NULL;
varbind = varbind->be_next) {
- if (ber_scanf_elements(varbind, "{oS}", ) != 0)
- goto fail;
+ if (ber_scanf_elements(varbind, "{oS}", ) != 0) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
+ break;
+ }
}
+ if (varbind != NULL)
+ continue;
ber_unlink_elements(message->be_sub->be_next);
ber_free_elements(message);
Re: ber.c: Don't continue on nonexistent ber
I found two issues related to this diff.
1) I posted a fix[0] for this one.
2) We can skip a NULL-ber on ')' and '}' since we replace it with a
parent ber.
There's only regress tests for ldapd and snmpd, so those are all I
tested.
martijn@
[0] https://marc.info/?l=openbsd-tech=156570803230850=2
On 8/13/19 3:37 PM, Claudio Jeker wrote:
> On Tue, Aug 13, 2019 at 03:27:17PM +0200, Martijn van Duren wrote:
>> I managed to make snmp(1) crash, when I sent a malformed snmp packet.
>> Specifically when I have a varbind with an oid, but no value.
>>
>> I test for this case via ber_scanf_elements("{oS}", which presumably
>> would crap out if my skip doesn't have an element. Unfortunately reality
>> is that the be_next is skipped and we try again with the same value.
>>
>> This can give us extremely weird results if we scan for two consecutive
>> elements of the same type (e.g. "ss") where the second element is
>> non-existent. This would result in the second element having the data
>> of the first element.
>>
>> Diff below fixes this.
>>
>> OK?
>
> If the various regress tests still work with this then OK claudio@
> It for sure makes sense but I wouldn't be surprised if there is code
> that expects this weird behaviour.
>
>> martijn@
>>
Index: ber.c
===
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.11
diff -u -p -r1.11 ber.c
--- ber.c 5 Aug 2019 12:38:14 - 1.11
+++ ber.c 13 Aug 2019 15:00:36 -
@@ -684,6 +684,8 @@ ber_scanf_elements(struct ber_element *b
va_start(ap, fmt);
while (*fmt) {
+ if (ber == NULL && *fmt != '}' && *fmt != ')')
+ goto fail;
switch (*fmt++) {
case 'B':
ptr = va_arg(ap, void **);
@@ -788,8 +790,6 @@ ber_scanf_elements(struct ber_element *b
goto fail;
}
- if (ber->be_next == NULL)
- continue;
ber = ber->be_next;
}
va_end(ap);
snmpd: fix traphandler
The traphandler currently relies on some false assumptions.
1) A pdu has 3 leading elements to the varbind list, not 4.
2) The first element of a trap varbind as 2 elements, not 3
3) The varbind list is optional.
The final point also causes "trap handle" in snmpd to print the
trap oid twice if no additional elements are send.
OK?
martijn@
Index: traphandler.c
===
RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v
retrieving revision 1.13
diff -u -p -r1.13 traphandler.c
--- traphandler.c 11 May 2019 17:46:02 - 1.13
+++ traphandler.c 13 Aug 2019 14:49:11 -
@@ -239,10 +239,11 @@ traphandler_parse(char *buf, size_t n, s
break;
case SNMP_V2:
- if (ber_scanf_elements(elm, "{{e}}", ) == -1 ||
- ber_scanf_elements(elm, "{SdS}{So}e",
- uptime, trapoid, vbinds) == -1)
+ if (ber_scanf_elements(elm, "{SSS{e}}", ) == -1 ||
+ ber_scanf_elements(elm, "{Sd}{So}",
+ uptime, trapoid) == -1)
goto done;
+ *vbinds = elm->be_next->be_next;
break;
default:
Re: minor INSTALL.loongson tweaks
> Is suspend-resume not working on the lemote anymore? It works (or used to work) on the Yeeloong, not on the Gdium (different battery controller chip).
iked(8): fix NAT traversal with empty "local" setting
There seems to be an annoying bug in iked NAT traversal which leads to an iked
falsely seeing a NAT when the "local" IP is not explicitly set in the config,
as a result two ikeds will switch from port 500 to 4500 with the first
CREATE_CHILD_SA exchange.
The diff adds a new flag to the message and enables
udpencap based on whether the last message contained a NAT detection notify.
Ok?
Index: iked.h
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/iked.h,v
retrieving revision 1.122
diff -u -p -u -r1.122 iked.h
--- iked.h 12 Aug 2019 07:40:45 - 1.122
+++ iked.h 13 Aug 2019 13:16:32 -
@@ -514,6 +514,7 @@ struct iked_message {
int msg_valid;
int msg_natt;
int msg_natt_rcvd;
+ int msg_nat_detected;
int msg_error;
int msg_e;
struct iked_message *msg_parent;
Index: ikev2.c
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.172
diff -u -p -u -r1.172 ikev2.c
--- ikev2.c 12 Aug 2019 07:40:45 - 1.172
+++ ikev2.c 13 Aug 2019 13:18:30 -
@@ -855,7 +855,7 @@ ikev2_init_recv(struct iked *env, struct
if (!ikev2_msg_frompeer(msg))
return;
- if (sa->sa_udpencap && sa->sa_natt == 0 &&
+ if (sa && msg->msg_nat_detected && sa->sa_natt == 0 &&
(sock = ikev2_msg_getsocket(env,
sa->sa_local.addr_af, 1)) != NULL) {
/*
@@ -872,9 +872,10 @@ ikev2_init_recv(struct iked *env, struct
msg->msg_fd = sa->sa_fd = sock->sock_fd;
msg->msg_sock = sock;
sa->sa_natt = 1;
+ sa->sa_udpencap = 1;
- log_debug("%s: NAT detected, updated SA to "
- "peer %s local %s", __func__,
+ log_debug("%s: detected NAT, enabling UDP encapsulation,"
+ " updated SA to peer %s local %s", __func__,
print_host((struct sockaddr *)>sa_peer.addr, NULL, 0),
print_host((struct sockaddr *)>sa_local.addr, NULL, 0));
}
@@ -2439,6 +2440,11 @@ ikev2_resp_ike_sa_init(struct iked *env,
if (sa->sa_hdr.sh_initiator) {
log_debug("%s: called by initiator", __func__);
return (-1);
+ }
+ if (msg->msg_nat_detected && sa->sa_udpencap == 0) {
+ log_debug("%s: detected NAT, enabling UDP encapsulation",
+ __func__);
+ sa->sa_udpencap = 1;
}
if ((buf = ikev2_msg_init(env, ,
Index: ikev2_pld.c
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.72
diff -u -p -u -r1.72 ikev2_pld.c
--- ikev2_pld.c 12 Aug 2019 07:40:45 - 1.72
+++ ikev2_pld.c 13 Aug 2019 13:16:32 -
@@ -1023,18 +1023,12 @@ ikev2_pld_notify(struct iked *env, struc
if (ikev2_nat_detection(env, msg, md, sizeof(md), type) == -1)
return (-1);
if (memcmp(buf, md, len) != 0) {
- log_debug("%s: %s detected NAT, enabling "
- "UDP encapsulation", __func__,
+ log_debug("%s: %s detected NAT", __func__,
print_map(type, ikev2_n_map));
-
- /*
-* Enable UDP encapsulation of ESP packages if
-* the check detected NAT.
-*/
- if (msg->msg_sa != NULL)
- msg->msg_sa->sa_udpencap = 1;
+ msg->msg_parent->msg_nat_detected = 1;
/* Send keepalive, since we are behind a NAT-gw */
- if (type == IKEV2_N_NAT_DETECTION_DESTINATION_IP)
+ if (msg->msg_sa != NULL &&
+ type == IKEV2_N_NAT_DETECTION_DESTINATION_IP)
msg->msg_sa->sa_usekeepalive = 1;
}
print_hex(md, 0, sizeof(md));
Re: iked(8): add transport mode for childsas
Update: Having the use_transport_mode flag attached to the SA is
not the best idea, so now it is given down to the child SA as soon as
possible and then only looked up from there (and cleared in the parent).
A simple setup looks as follows:
For A (/etc/iked.conf):
ikev2 "test" active transport esp from A to B \
peer B
For B (/etc/iked.conf):
ikev2 "test" active transport esp from B to A \
peer A
If successful ipsecctl -ssa (on both hosts) shows:
esp transport from A to B spi 0xedf7b2e6 auth hmac-sha2-256 enc aes-256
esp transport from B to A spi 0xedf7b2e6 auth hmac-sha2-256 enc aes-256
ok?
Here's the diff:
Index: iked.conf.5
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/iked.conf.5,v
retrieving revision 1.55
diff -u -p -u -r1.55 iked.conf.5
--- iked.conf.5 11 May 2019 16:30:23 - 1.55
+++ iked.conf.5 13 Aug 2019 12:23:48 -
@@ -268,6 +268,15 @@ specifies that
.Xr ipcomp 4 ,
the IP Payload Compression protocol, is negotiated in addition to
encapsulation.
The optional compression is applied before packets are encapsulated.
+.It Op Ar tmode
+.Ar tmode
+describes the encapsulation mode to be used.
+Possible modes are
+.Ar tunnel
+and
+.Ar transport ;
+the default is
+.Ar tunnel .
.It Op Ar encap
.Ar encap
specifies the encapsulation protocol to be used.
@@ -277,15 +286,6 @@ and
.Ar ah ;
the default is
.Ar esp .
-.\" .It Op Ar tmode
-.\" .Ar tmode
-.\" describes the encapsulation mode to be used.
-.\" Possible modes are
-.\" .Ar tunnel
-.\" and
-.\" .Ar transport ;
-.\" the default is
-.\" .Ar tunnel .
.It Op Ar af
This policy only applies to endpoints of the specified address family
which can be either
Index: iked.h
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/iked.h,v
retrieving revision 1.122
diff -u -p -u -r1.122 iked.h
--- iked.h 12 Aug 2019 07:40:45 - 1.122
+++ iked.h 13 Aug 2019 12:23:48 -
@@ -251,6 +251,7 @@ struct iked_policy {
#define IKED_POLICY_QUICK 0x08
#define IKED_POLICY_SKIP0x10
#define IKED_POLICY_IPCOMP 0x20
+#define IKED_POLICY_TRANSPORT 0x40
int pol_refcnt;
@@ -465,6 +466,7 @@ struct iked_sa {
int sa_mobike; /* MOBIKE */
int sa_frag; /* fragmentation */
+ int sa_use_transport_mode; /* should be
reset */
struct iked_timersa_timer; /* SA timeouts */
#define IKED_IKE_SA_EXCHANGE_TIMEOUT300/* 5 minutes */
Index: ikev2.c
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.172
diff -u -p -u -r1.172 ikev2.c
--- ikev2.c 12 Aug 2019 07:40:45 - 1.172
+++ ikev2.c 13 Aug 2019 12:47:49 -
@@ -148,8 +148,12 @@ ssize_t ikev2_add_nat_detection(struct i
ssize_t ikev2_add_fragmentation(struct iked *, struct ibuf *,
struct ikev2_payload **, struct iked_message *, ssize_t);
+ssize_t ikev2_add_notify(struct iked *, struct ibuf *,
+ struct ikev2_payload **, ssize_t, struct iked_sa *, uint16_t);
ssize_t ikev2_add_mobike(struct iked *, struct ibuf *,
struct ikev2_payload **, ssize_t, struct iked_sa *);
+ssize_t ikev2_add_transport_mode(struct iked *, struct ibuf *,
+ struct ikev2_payload **, ssize_t, struct iked_sa *);
int ikev2_update_sa_addresses(struct iked *, struct iked_sa *);
int ikev2_resp_informational(struct iked *, struct iked_sa *,
struct iked_message *);
@@ -1238,10 +1242,13 @@ ikev2_init_ike_auth(struct iked *env, st
goto done;
}
- /* compression */
+ /* compression, transport mode */
if ((pol->pol_flags & IKED_POLICY_IPCOMP) &&
(len = ikev2_add_ipcompnotify(env, e, , len, sa)) == -1)
goto done;
+ if ((pol->pol_flags & IKED_POLICY_TRANSPORT) &&
+ (len = ikev2_add_transport_mode(env, e, , len, sa)) == -1)
+ goto done;
if (ikev2_next_payload(pld, len, IKEV2_PAYLOAD_SA) == -1)
goto done;
@@ -1685,8 +1692,9 @@ ikev2_add_ipcompnotify(struct iked *env,
}
ssize_t
-ikev2_add_mobike(struct iked *env, struct ibuf *e,
-struct ikev2_payload **pld, ssize_t len, struct iked_sa *sa)
+ikev2_add_notify(struct iked *env, struct ibuf *e,
+struct ikev2_payload **pld, ssize_t len, struct iked_sa *sa,
+uint16_t notify)
{
struct ikev2_notify *n;
uint8_t *ptr;
@@ -1702,13 +1710,27 @@ ikev2_add_mobike(struct iked *env, struc
n = (struct ikev2_notify *)ptr;
n->n_protoid = 0;
n->n_spisize = 0;
- n->n_type =
snmpd fix invalid error codes
mps_get{,next}req makes the false assumption that root is empty, but if
o_get fails there might be data in there. The following diff fixes the
issue reported earlier today for the failing mib.
.iso.org.dod.internet.mgmt.mib_2.interfaces.ifTable.ifEntry.ifInDiscards
Changes the snmp(1) output from:
$ snmp get -v2c -On -cpublic 127.0.0.1 ifInDiscards.2
snmp: get: Undefined error: 0
to
$ snmp get -v2c -On -cpublic 127.0.0.1 ifInDiscards.2
.1.3.6.1.2.1.2.2.1.13.2 = No Such Object available on this agent at this OID
Also tested with getnext and bulkget.
OK?
martijn@
Index: mps.c
===
RCS file: /cvs/src/usr.sbin/snmpd/mps.c,v
retrieving revision 1.25
diff -u -p -r1.25 mps.c
--- mps.c 16 May 2019 05:00:00 - 1.25
+++ mps.c 13 Aug 2019 13:51:16 -
@@ -166,7 +166,11 @@ fail:
return (-1);
/* Set SNMPv2 extended error response. */
- elm = ber_add_oid(elm, o);
+ if (root->be_union.bv_sub != NULL) {
+ elm = ber_unlink_elements(root);
+ ber_free_elements(elm);
+ }
+ elm = ber_add_oid(root, o);
elm = ber_add_null(elm);
ber_set_header(elm, BER_CLASS_CONTEXT, error_type);
return (0);
@@ -289,7 +293,11 @@ fail:
return (-1);
/* Set SNMPv2 extended error response. */
- ber = ber_add_oid(ber, o);
+ if (root->be_union.bv_sub != NULL) {
+ ber = ber_unlink_elements(root);
+ ber_free_elements(ber);
+ }
+ ber = ber_add_oid(root, o);
ber = ber_add_null(ber);
ber_set_header(ber, BER_CLASS_CONTEXT, error_type);
return (0);
Re: iwm(4): fix ccmp decrypt edge cases
On 13/08/19(Tue) 14:58, Stefan Sperling wrote: > On Tue, Aug 13, 2019 at 09:40:22AM -0300, Martin Pieuchot wrote: > > On 13/08/19(Tue) 13:52, Stefan Sperling wrote: > > > This should hopefully prevent a crash reported to me by Jesper Wallin, > > > where net80211 crashes when it attempts to decrypt a CCMP-encrypted > > > frame which iwm passed up without decrypting it first. > > > > How does the stack crashes? > > Jesper only sent me a screen shot and no public bug report :( > > The trace looks roughly like this, where AES_Encrypt_ECB crashes because > ieee80211_ccmp_set_key() is never called since iwm_set_key overrides it. Could we call ieee80211_ccmp_set_key() with a default value when initializing the stack then? Would that prevent all the possible family of bugs? What problems can occur when passing encrypted frames to the stack without having configure a CCMP key? Are they packets going to be dropped at some point? > AES_Encrypt_ECB() > ieee80211_ccmp_phase1() > ieee80211_ccmp_decrypt() > ieee80211_input() > ieee80211_ba_move_window() > ieee80211_input() > iwm_rx_rx_mpdu() > iwm_notif_intr() > iwm_intr() > > What's a bit confusing here is that the interrupt was for an uencrypted > frame (a Block Ack Request) which moved the BA window forward. When > this happens we flush frames currently waiting in the Rx BA reordering > buffer. The encrypted frame which caused the crash was on that queue; > It must have passed through iwm_rx_rx_mpdu() earlier before the WPA > handshake was complete, and ended up waiting in the Rx BA queue because > its sequence number was off. > > Shouldn't we drop this packet in the stack as well? > > net80211 will always see a 'protected' bit in the frame header > In ieee80211_input, if rxi has an HW_DEC flag set, the code assumes > this frame needs no further processing for crypto. > Otherwise it tries to decrypt. > > So if the driver passes up a frame without decrypting it and without > setting HW_DEC, we crash. There's nothing that would allow net80211 > to detect an incorrect absence of HW_DEC. The driver overrides the > set_key function and corresponding frames are expected to arrive with > the HW_DEC flag set in net80211. (Please don't ask me to change this > design right now for a better fix, it wasn't my idea...) See above, if the crash is because of a missing key, put one :)
Re: ber.c: Don't continue on nonexistent ber
On Tue, Aug 13, 2019 at 03:27:17PM +0200, Martijn van Duren wrote:
> I managed to make snmp(1) crash, when I sent a malformed snmp packet.
> Specifically when I have a varbind with an oid, but no value.
>
> I test for this case via ber_scanf_elements("{oS}", which presumably
> would crap out if my skip doesn't have an element. Unfortunately reality
> is that the be_next is skipped and we try again with the same value.
>
> This can give us extremely weird results if we scan for two consecutive
> elements of the same type (e.g. "ss") where the second element is
> non-existent. This would result in the second element having the data
> of the first element.
>
> Diff below fixes this.
>
> OK?
If the various regress tests still work with this then OK claudio@
It for sure makes sense but I wouldn't be surprised if there is code
that expects this weird behaviour.
> martijn@
>
> Index: ber.c
> ===
> RCS file: /cvs/src/lib/libutil/ber.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 ber.c
> --- ber.c 5 Aug 2019 12:38:14 - 1.11
> +++ ber.c 13 Aug 2019 13:26:09 -
> @@ -684,6 +684,8 @@ ber_scanf_elements(struct ber_element *b
>
> va_start(ap, fmt);
> while (*fmt) {
> + if (ber == NULL)
> + goto fail;
> switch (*fmt++) {
> case 'B':
> ptr = va_arg(ap, void **);
> @@ -788,8 +790,6 @@ ber_scanf_elements(struct ber_element *b
> goto fail;
> }
>
> - if (ber->be_next == NULL)
> - continue;
> ber = ber->be_next;
> }
> va_end(ap);
>
--
:wq Claudio
ber.c: Don't continue on nonexistent ber
I managed to make snmp(1) crash, when I sent a malformed snmp packet.
Specifically when I have a varbind with an oid, but no value.
I test for this case via ber_scanf_elements("{oS}", which presumably
would crap out if my skip doesn't have an element. Unfortunately reality
is that the be_next is skipped and we try again with the same value.
This can give us extremely weird results if we scan for two consecutive
elements of the same type (e.g. "ss") where the second element is
non-existent. This would result in the second element having the data
of the first element.
Diff below fixes this.
OK?
martijn@
Index: ber.c
===
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.11
diff -u -p -r1.11 ber.c
--- ber.c 5 Aug 2019 12:38:14 - 1.11
+++ ber.c 13 Aug 2019 13:26:09 -
@@ -684,6 +684,8 @@ ber_scanf_elements(struct ber_element *b
va_start(ap, fmt);
while (*fmt) {
+ if (ber == NULL)
+ goto fail;
switch (*fmt++) {
case 'B':
ptr = va_arg(ap, void **);
@@ -788,8 +790,6 @@ ber_scanf_elements(struct ber_element *b
goto fail;
}
- if (ber->be_next == NULL)
- continue;
ber = ber->be_next;
}
va_end(ap);
Re: iwm(4): fix ccmp decrypt edge cases
On Tue, Aug 13, 2019 at 09:40:22AM -0300, Martin Pieuchot wrote: > On 13/08/19(Tue) 13:52, Stefan Sperling wrote: > > This should hopefully prevent a crash reported to me by Jesper Wallin, > > where net80211 crashes when it attempts to decrypt a CCMP-encrypted > > frame which iwm passed up without decrypting it first. > > How does the stack crashes? Jesper only sent me a screen shot and no public bug report :( The trace looks roughly like this, where AES_Encrypt_ECB crashes because ieee80211_ccmp_set_key() is never called since iwm_set_key overrides it. AES_Encrypt_ECB() ieee80211_ccmp_phase1() ieee80211_ccmp_decrypt() ieee80211_input() ieee80211_ba_move_window() ieee80211_input() iwm_rx_rx_mpdu() iwm_notif_intr() iwm_intr() What's a bit confusing here is that the interrupt was for an uencrypted frame (a Block Ack Request) which moved the BA window forward. When this happens we flush frames currently waiting in the Rx BA reordering buffer. The encrypted frame which caused the crash was on that queue; It must have passed through iwm_rx_rx_mpdu() earlier before the WPA handshake was complete, and ended up waiting in the Rx BA queue because its sequence number was off. > Shouldn't we drop this packet in the stack as well? net80211 will always see a 'protected' bit in the frame header In ieee80211_input, if rxi has an HW_DEC flag set, the code assumes this frame needs no further processing for crypto. Otherwise it tries to decrypt. So if the driver passes up a frame without decrypting it and without setting HW_DEC, we crash. There's nothing that would allow net80211 to detect an incorrect absence of HW_DEC. The driver overrides the set_key function and corresponding frames are expected to arrive with the HW_DEC flag set in net80211. (Please don't ask me to change this design right now for a better fix, it wasn't my idea...) > > By code inspection I have determined that this problem could happen > > in case a CCMP frame is received peer before the WPA handshake has > > completed (e.g. during re-association). Note how the hardware decryption > > code path depends on IEEE80211_NODE_RXPROT being set, which is set only > > once the handshake has been completed. > > We have to be careful here to avoid breaking non-CCMP ciphers (WEP, TKIP). > > So it's like working around the firwmare by defining two new state > related to the CCMP key and dropping packets that should not reach > us at this point? Yes. > > It also attempts to close a small race where we're handing the key to > > firmware and eventually get an interrupt which confirms key installation, > > in parallel to sending/receiving unicast data frames. > > Until the key has been confirmed by firmware, don't transmit data frames > > and drop received encrypted frames. > > Makes sense. > > The actual solution also has a race if you change the key before > acknowledging the firmware IWM_ADD_STA_KEY, but I suppose this is > acceptable. The stack-side key has already been changed at that point. It is derived from the WPA handshake. The firmware has to accept it or we can't proceed. > > This part of the diff has poor error handling; if firmware never confirms > > they key then iwm will get stuck and the user will have to recover. > > Well up/down should work, right? Yes it should. > > But that should be better than the current situation where we'd be > > sending unencrypted frames or perhaps crash in net80211. > > > > These fixes could be separated but a combined diff is easier to test. > > What should we test? Just check for regressions. I have lightly tested it on one machine and it works there. There's not a lot of testing needed, but if a few people could run the diff to check if I've missed something, that would be nice.
Re: iwm(4): fix ccmp decrypt edge cases
On 13/08/19(Tue) 13:52, Stefan Sperling wrote:
> This should hopefully prevent a crash reported to me by Jesper Wallin,
> where net80211 crashes when it attempts to decrypt a CCMP-encrypted
> frame which iwm passed up without decrypting it first.
How does the stack crashes? Shouldn't we drop this packet in the stack
as well?
> By code inspection I have determined that this problem could happen
> in case a CCMP frame is received peer before the WPA handshake has
> completed (e.g. during re-association). Note how the hardware decryption
> code path depends on IEEE80211_NODE_RXPROT being set, which is set only
> once the handshake has been completed.
> We have to be careful here to avoid breaking non-CCMP ciphers (WEP, TKIP).
So it's like working around the firwmare by defining two new state
related to the CCMP key and dropping packets that should not reach
us at this point?
> It also attempts to close a small race where we're handing the key to
> firmware and eventually get an interrupt which confirms key installation,
> in parallel to sending/receiving unicast data frames.
> Until the key has been confirmed by firmware, don't transmit data frames
> and drop received encrypted frames.
Makes sense.
The actual solution also has a race if you change the key before
acknowledging the firmware IWM_ADD_STA_KEY, but I suppose this is
acceptable.
> This part of the diff has poor error handling; if firmware never confirms
> they key then iwm will get stuck and the user will have to recover.
Well up/down should work, right?
> But that should be better than the current situation where we'd be
> sending unencrypted frames or perhaps crash in net80211.
>
> These fixes could be separated but a combined diff is easier to test.
What should we test?
> Once we've got this working for iwm(4), the iwn(4) driver will need
> a similar fix.
>
> diff refs/heads/master refs/heads/ccmp-crash
> blob - 038e6a63dfff113b525bb1e9a30c935996535569
> blob + 0571a3a042c0097002241e2accc026c55af205ed
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -3601,6 +3601,13 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
> !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
> (ni->ni_flags & IEEE80211_NODE_RXPROT) &&
> ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) {
> + if ((sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) {
> + /* Firmware has not installed the key yet. */
> + ic->ic_stats.is_ccmp_dec_errs++;
> + ifp->if_ierrors++;
> + m_freem(m);
> + goto done;
> + }
> if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) !=
> IWM_RX_MPDU_RES_STATUS_SEC_CCM_ENC) {
> ic->ic_stats.is_ccmp_dec_errs++;
> @@ -3625,6 +3632,22 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
> goto done;
> }
> rxi.rxi_flags |= IEEE80211_RXI_HWDEC;
> + } else if ((wh->i_fc[1] & IEEE80211_FC1_PROTECTED) &&
> + !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
Bikesheding: these 5 hardware decryption checks are executed for
non-multicast and if the FC1_PROTECTED bit is set right? Should
that be a single if statement?
> + (ic->ic_flags & IEEE80211_F_RSNON) &&
> + (ic->ic_rsnprotos & IEEE80211_PROTO_RSN) &&
> + (ic->ic_rsnciphers & IEEE80211_CIPHER_CCMP) &&
> + (ni->ni_rsnciphers & IEEE80211_CIPHER_CCMP) &&
> + (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0) {
> + /*
> + * We're doing CCMP and have received an encrypted unicast
> + * frame before 4-way handshake has completed. Don't pass it
> + * up the stack; net80211 would crash trying to decrypt it.
> + */
> + ic->ic_stats.is_ccmp_dec_errs++;
> + ifp->if_ierrors++;
> + m_freem(m);
> + goto done;
> }
>
> #if NBPFILTER > 0
> @@ -6090,6 +6113,7 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
> {
> struct iwm_softc *sc = ic->ic_softc;
> struct iwm_add_sta_key_cmd cmd = { 0 };
> + int err;
>
> if ((k->k_flags & IEEE80211_KEY_GROUP) ||
> k->k_cipher != IEEE80211_CIPHER_CCMP) {
> @@ -6097,6 +6121,8 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
> return (ieee80211_set_key(ic, ni, k));
> }
>
> + sc->sc_flags &= ~IWM_FLAG_CCMP_READY;
> +
> cmd.key_flags = htole16(IWM_STA_KEY_FLG_CCM |
> IWM_STA_KEY_FLG_WEP_KEY_MAP |
> ((k->k_id << IWM_STA_KEY_FLG_KEYID_POS) &
> @@ -6108,8 +6134,11 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
> cmd.key_offset = 0;
> cmd.sta_id = IWM_STATION_ID;
>
> - return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
> + err = iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
> sizeof(cmd), );
> + if
iwm(4): fix ccmp decrypt edge cases
This should hopefully prevent a crash reported to me by Jesper Wallin,
where net80211 crashes when it attempts to decrypt a CCMP-encrypted
frame which iwm passed up without decrypting it first.
By code inspection I have determined that this problem could happen
in case a CCMP frame is received peer before the WPA handshake has
completed (e.g. during re-association). Note how the hardware decryption
code path depends on IEEE80211_NODE_RXPROT being set, which is set only
once the handshake has been completed.
We have to be careful here to avoid breaking non-CCMP ciphers (WEP, TKIP).
It also attempts to close a small race where we're handing the key to
firmware and eventually get an interrupt which confirms key installation,
in parallel to sending/receiving unicast data frames.
Until the key has been confirmed by firmware, don't transmit data frames
and drop received encrypted frames.
This part of the diff has poor error handling; if firmware never confirms
they key then iwm will get stuck and the user will have to recover.
But that should be better than the current situation where we'd be
sending unencrypted frames or perhaps crash in net80211.
These fixes could be separated but a combined diff is easier to test.
Once we've got this working for iwm(4), the iwn(4) driver will need
a similar fix.
diff refs/heads/master refs/heads/ccmp-crash
blob - 038e6a63dfff113b525bb1e9a30c935996535569
blob + 0571a3a042c0097002241e2accc026c55af205ed
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -3601,6 +3601,13 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
!IEEE80211_IS_MULTICAST(wh->i_addr1) &&
(ni->ni_flags & IEEE80211_NODE_RXPROT) &&
ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) {
+ if ((sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) {
+ /* Firmware has not installed the key yet. */
+ ic->ic_stats.is_ccmp_dec_errs++;
+ ifp->if_ierrors++;
+ m_freem(m);
+ goto done;
+ }
if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) !=
IWM_RX_MPDU_RES_STATUS_SEC_CCM_ENC) {
ic->ic_stats.is_ccmp_dec_errs++;
@@ -3625,6 +3632,22 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
goto done;
}
rxi.rxi_flags |= IEEE80211_RXI_HWDEC;
+ } else if ((wh->i_fc[1] & IEEE80211_FC1_PROTECTED) &&
+ !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
+ (ic->ic_flags & IEEE80211_F_RSNON) &&
+ (ic->ic_rsnprotos & IEEE80211_PROTO_RSN) &&
+ (ic->ic_rsnciphers & IEEE80211_CIPHER_CCMP) &&
+ (ni->ni_rsnciphers & IEEE80211_CIPHER_CCMP) &&
+ (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0) {
+ /*
+* We're doing CCMP and have received an encrypted unicast
+* frame before 4-way handshake has completed. Don't pass it
+* up the stack; net80211 would crash trying to decrypt it.
+*/
+ ic->ic_stats.is_ccmp_dec_errs++;
+ ifp->if_ierrors++;
+ m_freem(m);
+ goto done;
}
#if NBPFILTER > 0
@@ -6090,6 +6113,7 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
{
struct iwm_softc *sc = ic->ic_softc;
struct iwm_add_sta_key_cmd cmd = { 0 };
+ int err;
if ((k->k_flags & IEEE80211_KEY_GROUP) ||
k->k_cipher != IEEE80211_CIPHER_CCMP) {
@@ -6097,6 +6121,8 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
return (ieee80211_set_key(ic, ni, k));
}
+ sc->sc_flags &= ~IWM_FLAG_CCMP_READY;
+
cmd.key_flags = htole16(IWM_STA_KEY_FLG_CCM |
IWM_STA_KEY_FLG_WEP_KEY_MAP |
((k->k_id << IWM_STA_KEY_FLG_KEYID_POS) &
@@ -6108,8 +6134,11 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
cmd.key_offset = 0;
cmd.sta_id = IWM_STATION_ID;
- return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
+ err = iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
sizeof(cmd), );
+ if (!err)
+ sc->sc_flags |= IWM_FLAG_CCMP_KEY_SENT;
+ return err;
}
void
@@ -6135,6 +6164,7 @@ iwm_delete_key(struct ieee80211com *ic, struct ieee802
cmd.sta_id = IWM_STATION_ID;
iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC, sizeof(cmd), );
+ sc->sc_flags &= ~(IWM_FLAG_CCMP_KEY_SENT | IWM_FLAG_CCMP_READY);
}
void
@@ -6765,6 +6795,14 @@ iwm_start(struct ifnet *ifp)
(ic->ic_xflags & IEEE80211_F_TX_MGMT_ONLY))
break;
+ if (sc->sc_flags & IWM_FLAG_CCMP_KEY_SENT) {
+ /*
+* Can't send data frames before firmware is
+* ready to encrypt them.
+
bgpd use getpeerbyid() instead of dumb loop
When finding an peer id for a new templated host getpeerbyip() uses a
rather dumb lookup loop which is super inefficent. Instead it is much
better to just use getpeerbyid() and check its return.
Also while there don't use the global conf for the peer list but instead
use the argument c in all RB functions for all getpeer* functions.
OK?
--
:wq Claudio
Index: session.c
===
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
retrieving revision 1.389
diff -u -p -r1.389 session.c
--- session.c 12 Aug 2019 14:15:27 - 1.389
+++ session.c 13 Aug 2019 08:25:49 -
@@ -2894,7 +2894,7 @@ getpeerbydesc(struct bgpd_config *c, con
struct peer *p, *res = NULL;
int match = 0;
- RB_FOREACH(p, peer_head, >peers)
+ RB_FOREACH(p, peer_head, >peers)
if (!strcmp(p->conf.descr, descr)) {
res = p;
match++;
@@ -2920,13 +2920,13 @@ getpeerbyip(struct bgpd_config *c, struc
sa2addr(ip, , NULL);
/* we might want a more effective way to find peers by IP */
- RB_FOREACH(p, peer_head, >peers)
+ RB_FOREACH(p, peer_head, >peers)
if (!p->conf.template &&
!memcmp(, >conf.remote_addr, sizeof(addr)))
return (p);
/* try template matching */
- RB_FOREACH(p, peer_head, >peers)
+ RB_FOREACH(p, peer_head, >peers)
if (p->conf.template &&
p->conf.remote_addr.aid == addr.aid &&
session_match_mask(p, ))
@@ -2940,10 +2940,7 @@ getpeerbyip(struct bgpd_config *c, struc
fatal(NULL);
memcpy(newpeer, loose, sizeof(struct peer));
for (id = PEER_ID_DYN_MAX; id > PEER_ID_STATIC_MAX; id--) {
- RB_FOREACH(p, peer_head, >peers)
- if (p->conf.id == id)
- break;
- if (p == NULL) /* we found a free id */
+ if (getpeerbyid(c, id) == NULL) /* we found a free id */
break;
}
newpeer->template = loose;
bgpd rde_filter() change
When adding the filterstate to rde_filter I also passed a struct prefix
pointer to rde_filter instead of passing the 4 values. This resulted in
some ugly hacks because in some cases there was no prefix handy to pass
in and while working on RIB pipelines I noticed that this is hurting me
again. So time to change it and just pass the prefix_peer, prefix_vstate
and the prefix addr/len right into rde_filter().
While there I also reordered the args to rde_attr_set() to match
rde_filter(). This is mostly a mechanical change.
OK?
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.484
diff -u -p -r1.484 rde.c
--- rde.c 9 Aug 2019 13:44:27 - 1.484
+++ rde.c 13 Aug 2019 07:50:31 -
@@ -1402,7 +1402,6 @@ rde_update_update(struct rde_peer *peer,
struct bgpd_addr *prefix, u_int8_t prefixlen)
{
struct filterstate state;
- struct prefix *p;
enum filter_actions action;
u_int8_t vstate;
u_int16_ti;
@@ -1428,17 +1427,14 @@ rde_update_update(struct rde_peer *peer,
if (in->aspath.flags & F_ATTR_PARSE_ERR)
wmsg = "path invalid, withdraw";
- p = prefix_get([RIB_ADJ_IN].rib, peer, prefix, prefixlen);
- if (p == NULL)
- fatalx("rde_update_update: no prefix in Adj-RIB-In");
-
for (i = RIB_LOC_START; i < rib_size; i++) {
if (!rib_valid(i))
continue;
rde_filterstate_prep(, >aspath, >communities,
in->nexthop, in->nhflags);
/* input filter */
- action = rde_filter(ribs[i].in_rules, peer, p, );
+ action = rde_filter(ribs[i].in_rules, peer, peer, prefix,
+ prefixlen, vstate, );
if (action == ACTION_ALLOW) {
rde_update_log("update", i, peer,
@@ -3327,7 +3323,8 @@ rde_softreconfig_in(struct rib_entry *re
rde_filterstate_prep(, asp, prefix_communities(p),
prefix_nexthop(p), prefix_nhflags(p));
- action = rde_filter(rib->in_rules, peer, p, );
+ action = rde_filter(rib->in_rules, peer, peer, ,
+ pt->prefixlen, p->validation_state, );
if (action == ACTION_ALLOW) {
/* update Local-RIB */
@@ -3959,10 +3956,10 @@ network_add(struct network_config *nc, s
}
}
- rde_apply_set(>attrset, state, nc->prefix.aid, peerself, peerself);
+ rde_apply_set(>attrset, peerself, peerself, state, nc->prefix.aid);
if (vpnset)
- rde_apply_set(vpnset, state, nc->prefix.aid, peerself,
- peerself);
+ rde_apply_set(vpnset, peerself, peerself, state,
+ nc->prefix.aid);
vstate = rde_roa_validity(>rde_roa, >prefix,
nc->prefixlen, aspath_origin(state->aspath.aspath));
Index: rde.h
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.223
diff -u -p -r1.223 rde.h
--- rde.h 9 Aug 2019 13:44:27 - 1.223
+++ rde.h 12 Aug 2019 14:46:38 -
@@ -464,16 +464,17 @@ intcommunity_to_rd(struct community *,
voidprefix_evaluate(struct prefix *, struct rib_entry *);
/* rde_filter.c */
-voidrde_filterstate_prep(struct filterstate *, struct rde_aspath *,
-struct rde_community *, struct nexthop *, u_int8_t);
-voidrde_filterstate_clean(struct filterstate *);
+void rde_apply_set(struct filter_set_head *, struct rde_peer *,
+ struct rde_peer *, struct filterstate *, u_int8_t);
+void rde_filterstate_prep(struct filterstate *, struct rde_aspath *,
+ struct rde_community *, struct nexthop *, u_int8_t);
+void rde_filterstate_clean(struct filterstate *);
+intrde_filter_equal(struct filter_head *, struct filter_head *,
+ struct rde_peer *);
+void rde_filter_calc_skip_steps(struct filter_head *);
enum filter_actions rde_filter(struct filter_head *, struct rde_peer *,
-struct prefix *, struct filterstate *);
-voidrde_apply_set(struct filter_set_head *, struct filterstate *,
-u_int8_t, struct rde_peer *, struct rde_peer *);
-int rde_filter_equal(struct filter_head *, struct filter_head *,
-struct rde_peer *);
-voidrde_filter_calc_skip_steps(struct filter_head *);
+ struct rde_peer *, struct bgpd_addr *, u_int8_t, u_int8_t,
+ struct filterstate *);
/* rde_prefix.c */
voidpt_init(void);
Index: rde_filter.c
===
RCS file:
Re: iked(8): improve logging output
On Fri, Aug 09, 2019 at 05:42:30PM +0200, Reyk Floeter wrote: > Hi, > > I agree that __func__ should be removed from anything except log_debug() > messages. > > I think you should prepend the term sa or spi to explain what the hex numbers > mean. > > otherwise OK reyk Thanks! Added with "spi=" prepended to the spi values.
