"James MacMahon" <[email protected]> writes:
> Hello,
Hi, thanks for your report,
> (sendbug didn't work for me, so I am sending using the same format manually)
>
> Description:
> Recently, I found an OpenBSD bug where pppoe(4) sends echo-req packets with
> bad
> magic numbers. The following tcpdump trace below shows the bad echo-req packet
> with 00-00-00-00:
>
> Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp output <echo-req id=0xea len=8
> 00-00-00-00>
> Nov 1 09:24:59 smyslov /bsd: pppoe0 (8864) state=3, session=0x3d22 output ->
> 00:90:1a:a3:7e:12, len=16
> Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp input(opened): <term-req id=0xbf
> len=4 00-00-00-...-00-00-00>
> Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp opened->stopping
>
> According to RFC 1661, "A Magic-Number of zero is illegal and MUST always be
> Nak'd".
>
> The problem exists in sys/net/if_spppsubr.c:
>
> else if (sp->pp_phase >= PHASE_AUTHENTICATE) {
> unsigned long nmagic = htonl (sp->lcp.magic);
> sp->lcp.echoid = ++sp->pp_seq;
> sppp_cp_send (sp, PPP_LCP, ECHO_REQ,
> sp->lcp.echoid, 4, &nmagic);
> }
>
> The problem here is that sparc64 is big endian and 64 bit, and passing the
> address using '&nmagic' causes sppp_cp_send to see only zeros in the 'first'
> four bytes (most significant bytes).
[...]
Here's a diff, only compile-tested, to treat "magic" as u_int32_t.
I didn't switch "magic" in struct slcp to u_int32_t, that can be done in
another diff (and for other members of this structure) if someone cares.
If this looks right I'd like some reports (LE/BE, ILP32/I32LP64).
Index: if_spppsubr.c
===================================================================
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.113
diff -u -p -r1.113 if_spppsubr.c
--- if_spppsubr.c 20 Nov 2013 08:21:33 -0000 1.113
+++ if_spppsubr.c 26 Nov 2013 00:50:45 -0000
@@ -1312,7 +1312,7 @@ sppp_cp_input(const struct cp *cp, struc
int len = m->m_pkthdr.len;
int rv;
u_char *p;
- u_long nmagic;
+ u_int32_t magic, nmagic;
if (len < 4) {
if (debug)
@@ -1641,10 +1641,10 @@ sppp_cp_input(const struct cp *cp, struc
break;
}
- nmagic = (u_long)p[0] << 24 |
- (u_long)p[1] << 16 | p[2] << 8 | p[3];
+ memcpy(&nmagic, p, sizeof(nmagic));
+ magic = ntohl(nmagic);
- if (nmagic == sp->lcp.magic) {
+ if (magic == sp->lcp.magic) {
/* Line loopback mode detected. */
log(LOG_INFO, SPP_FMT "loopback\n", SPP_ARGS(ifp));
/* Shut down the PPP link. */
@@ -1652,10 +1652,8 @@ sppp_cp_input(const struct cp *cp, struc
break;
}
- p[0] = sp->lcp.magic >> 24;
- p[1] = sp->lcp.magic >> 16;
- p[2] = sp->lcp.magic >> 8;
- p[3] = sp->lcp.magic;
+ nmagic = htonl(sp->lcp.magic);
+ memcpy(p, &nmagic, sizeof(nmagic));
if (debug)
addlog(SPP_FMT "got lcp echo req, sending echo rep\n",
@@ -1679,11 +1677,11 @@ sppp_cp_input(const struct cp *cp, struc
if (debug)
addlog(SPP_FMT "lcp got echo rep\n",
SPP_ARGS(ifp));
-
- nmagic = (u_long)p[0] << 24 |
- (u_long)p[1] << 16 | p[2] << 8 | p[3];
- if (nmagic != sp->lcp.magic)
+ memcpy(&nmagic, p, sizeof(nmagic));
+ magic = ntohl(nmagic);
+
+ if (magic != sp->lcp.magic)
sp->pp_alivecnt = 0;
break;
default:
@@ -2109,7 +2107,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
STDDCL;
u_char *buf, *r, *p;
int origlen, rlen;
- u_long nmagic;
+ u_int32_t magic, nmagic;
u_short authproto;
len -= 4;
@@ -2209,11 +2207,11 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
switch (*p) {
case LCP_OPT_MAGIC:
/* Magic number -- extract. */
- nmagic = (u_long)p[2] << 24 |
- (u_long)p[3] << 16 | p[4] << 8 | p[5];
- if (nmagic != sp->lcp.magic) {
+ memcpy(&nmagic, &p[2], sizeof(nmagic));
+ magic = ntohl(nmagic);
+ if (magic != sp->lcp.magic) {
if (debug)
- addlog("0x%lx ", nmagic);
+ addlog("0x%08x ", magic);
continue;
}
if (debug)
@@ -2224,12 +2222,9 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
* we see it later in an NAK packet, we
* suggest a new one.
*/
- nmagic = ~sp->lcp.magic;
+ nmagic = ~sp->lcp.magic & 0xffffffffU;
/* Gonna NAK it. */
- p[2] = nmagic >> 24;
- p[3] = nmagic >> 16;
- p[4] = nmagic >> 8;
- p[5] = nmagic;
+ memcpy(&p[2], &nmagic, sizeof(nmagic));
break;
case LCP_OPT_ASYNC_MAP:
@@ -2376,7 +2371,7 @@ sppp_lcp_RCN_nak(struct sppp *sp, struct
{
STDDCL;
u_char *p;
- u_long magic;
+ u_int32_t magic, nmagic;
len -= 4;
@@ -2395,21 +2390,22 @@ sppp_lcp_RCN_nak(struct sppp *sp, struct
/* Magic number -- renegotiate */
if ((sp->lcp.opts & (1 << LCP_OPT_MAGIC)) &&
len >= 6 && p[1] == 6) {
- magic = (u_long)p[2] << 24 |
- (u_long)p[3] << 16 | p[4] << 8 | p[5];
+ memcpy(&nmagic, &p[2], sizeof(nmagic));
+ magic = ntohl(nmagic);
+
/*
* If the remote magic is our negated one,
* this looks like a loopback problem.
* Suggest a new magic to make sure.
*/
- if (magic == ~sp->lcp.magic) {
+ if (magic == (~sp->lcp.magic & 0xffffffffU)) {
if (debug)
addlog("magic glitch ");
sp->lcp.magic = arc4random();
} else {
sp->lcp.magic = magic;
if (debug)
- addlog("%lu ", magic);
+ addlog("0x%08x ", magic);
}
}
break;
@@ -2557,6 +2553,7 @@ sppp_lcp_scr(struct sppp *sp)
{
char opt[6 /* magicnum */ + 4 /* mru */ + 5 /* chap */];
int i = 0;
+ u_int32_t nmagic;
u_short authproto;
if (sp->lcp.opts & (1 << LCP_OPT_MAGIC)) {
@@ -2564,10 +2561,9 @@ sppp_lcp_scr(struct sppp *sp)
sp->lcp.magic = arc4random();
opt[i++] = LCP_OPT_MAGIC;
opt[i++] = 6;
- opt[i++] = sp->lcp.magic >> 24;
- opt[i++] = sp->lcp.magic >> 16;
- opt[i++] = sp->lcp.magic >> 8;
- opt[i++] = sp->lcp.magic;
+ nmagic = htonl(sp->lcp.magic);
+ memcpy(&opt[i], &nmagic, sizeof(nmagic));
+ i += 4;
}
if (sp->lcp.opts & (1 << LCP_OPT_MRU)) {
@@ -4488,10 +4484,12 @@ sppp_keepalive(void *dummy)
sppp_cisco_send (sp, CISCO_KEEPALIVE_REQ, ++sp->pp_seq,
sp->pp_rseq);
else if (sp->pp_phase >= PHASE_AUTHENTICATE) {
- unsigned long nmagic = htonl (sp->lcp.magic);
+ u_int32_t nmagic;
+
+ nmagic = htonl(sp->lcp.magic);
sp->lcp.echoid = ++sp->pp_seq;
- sppp_cp_send (sp, PPP_LCP, ECHO_REQ,
- sp->lcp.echoid, 4, &nmagic);
+ sppp_cp_send(sp, PPP_LCP, ECHO_REQ, sp->lcp.echoid,
+ 4, &nmagic);
}
}
splx(s);
@@ -5223,13 +5221,6 @@ sppp_null(struct sppp *unused)
{
/* do just nothing */
}
-/*
- * This file is large. Tell emacs to highlight it nevertheless.
- *
- * Local Variables:
- * hilit-auto-highlight-maxout: 120000
- * End:
- */
HIDE void
sppp_set_phase(struct sppp *sp)
