RTM_DELETE messages for L2 routes have incorrect flags

2020-08-10 Thread Jonathan Matthew
While looking into filtering out messages for L2 routes in the kernel to reduce
load on routing daemons, I noticed that the RTM_DELETE messages do not have
the RTF_LLINFO flag set, which is inconvenient because that's what I want to
filter on.

I tracked this down to r1.361 and r1.362 of net/route.c, where we stopped
saving rt->rt_flags before calling rtrequest_delete().  rtrequest_delete()
calls ifp->if_rtrequest(), which removes the llinfo from the route and clears
RTF_LLINFO.

I think the simplest way to fix this would be for rtdeletemsg() to go back to
calling rtm_miss() directly rather than using rtm_send().  Adding more
parameters to rtm_send() to specify additional flags seems like
overcomplicating it.


Index: route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.394
diff -u -p -r1.394 route.c
--- route.c 24 Jun 2020 22:03:43 -  1.394
+++ route.c 11 Aug 2020 04:12:51 -
@@ -663,6 +663,7 @@ rtdeletemsg(struct rtentry *rt, struct i
 {
int error;
struct rt_addrinfo  info;
+   struct sockaddr_rtlabel sa_rl;
struct sockaddr_in6 sa_mask;
 
KASSERT(rt->rt_ifidx == ifp->if_index);
@@ -677,8 +678,13 @@ rtdeletemsg(struct rtentry *rt, struct i
info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
if (!ISSET(rt->rt_flags, RTF_HOST))
info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, _mask);
+   info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt->rt_labelid, _rl);
+   info.rti_flags = rt->rt_flags;
+   info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
+   info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
-   rtm_send(rt, RTM_DELETE, error, tableid);
+   rtm_miss(RTM_DELETE, , info.rti_flags, rt->rt_priority,
+   rt->rt_ifidx, error, tableid);
if (error == 0)
rtfree(rt);
return (error);



Re: Fwd: explicit_bzero vs. alternatives

2020-08-10 Thread Damien Miller
On Mon, 10 Aug 2020, Amit Kulkarni wrote:

> moving to tech@
> 
> -- Forwarded message -
> From: Philipp Klaus Krause 
> Date: Mon, Aug 10, 2020 at 4:34 AM
> Subject: explicit_bzero vs. alternatives
> To: 
> 
> 
> OpenBSD has the explicit_bzero function to reliably (i.e. even if not
> observable in the C abstract machine) overwrite memory with zeroes.
> 
> WG14 is currently considering adding similar functionality to C2X.
> 
> Considered options include:
> 
> * A function like explicit_bzero or memset_explicit, that overwrites the
> memory with a known value.
> * A function like memclear, that overwrites the memory in an
> implementation-defined manner, possibly using random data.
> 
> Is there a rationale why OpenBSD went with their explicit_bzero design?
> Were alternatives considered and rejected?

We went with explict_bzero because our only use-case for this was
safe erasure that could not be elided by the compiler.

I don't see any need for explicit_memset() - if anything depends on
the overwritten value then simple memset() should be sufficient as
the compiler should detect the dependency and refuse to elide the
memset() to begin with.

Likewise, I can see no benefit for overwriting with random data. Doing
this is always going to be more expensive and more likely to leak
secrets, e.g. the length of cleared objects.

Hopefully C2X is taking a more broad approach to this problem than
considering new library calls. Over-eager optimisation (especially when
done at link-time over the whole program) is a major for anyone trying
to write safe C code.

-d



Re: pipex "idle-timeout" work with pppx(4).

2020-08-10 Thread Vitaliy Makkoveev



> On 10 Aug 2020, at 19:53, Vitaliy Makkoveev  wrote:
> 
> We are doing all wrong :)
> 
> We can just unlink pppx(4) related session from `pipex_session_list' if
> it's time expired. But since this unlinked session is still exists in
> pppx(4) layer we can access through pppx_get_closed() without any
> search. We should only add flag to session which identifies it as
> pppx(4) related.
> 
> I hope you like this idea.
> 
>  cut begin 
> Static void
> pipex_timer(void *ignored_arg)
> {
>struct pipex_session *session, *session_tmp;
> 
>timeout_add_sec(_timer_ch, pipex_prune);
> 
>NET_LOCK();
>/* walk through */
>LIST_FOREACH_SAFE(session, _session_list, session_list,
>session_tmp) {
>switch (session->state) {
>case PIPEX_STATE_OPENED:
>if (session->timeout_sec == 0)
>continue;
> 
>session->stat.idle_time++;
>if (session->stat.idle_time < session->timeout_sec)
>continue;
> 
>   if (session->pppx_session)
>   pipex_unlink_session(session);
>   else
>   pipex_notify_close_session(session);
>break;
>   /* ... */
> }
> 
> pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> {
>   struct pppx_if *pxi;
> 
>   pxi = pppx_if_find(pxd, req->pdr_session_id, req->pdr_protocol);
>   if (pxi == NULL)
>   return (EINVAL);
> 
>   memset(req, 0, sizeof(*req));
>   if (session->state == PIPEX_STATE_CLOSED) {
>   req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
>   pppx_if_destroy(pxi);   
>   }
> 
>   return 0;
> }

Sorry for noise. I should avoid to write pseudo code.



Re: pms(4): disable parity checking for specific elantech fw

2020-08-10 Thread sxvghd

On 2020-08-08 22:47, Marcus Glocker wrote:

Can we maybe add a small comment explaining the inverted parity bits
behaviour on cold boot for this firmware version?


Sure thing.

Index: pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.93
diff -u -p -u -r1.93 pms.c
--- pms.c   4 Jul 2020 10:39:25 -   1.93
+++ pms.c   10 Aug 2020 12:58:01 -
@@ -2292,7 +2292,12 @@ pms_sync_elantech_v1(struct pms_softc *s
}

if (data < 0 || data >= nitems(elantech->parity) ||
-   elantech->parity[data] != p)
+   /*
+* FW 0x20022 sends inverted parity bits on cold boot, returning
+* to normal after suspend & resume, so the parity check is
+* disabled for this one.
+*/
+   (elantech->fw_version != 0x20022 && elantech->parity[data] != p))
return (-1);

return (0);



Re: [Patch] Change httpd's handling of request "Host:" headers

2020-08-10 Thread Sebastian Benoit
Ross L Richardson(open...@rlr.id.au) on 2020.08.09 20:07:11 +1000:
> 
> At present, if a request contains no "Host:" header [HTTP pre-1.1] or
> if the supplied header does not match any of the servers configured
> in httpd.conf, the request is directed to the first server.  This
> isn't documented, AFAICT.
> 
> For example, if httpd.conf has just one server
>   server "www.example.com"
> then we currently get
>   $ printf "HEAD / HTTP/1.0\r\nHost: www.openbsd.org\r\n\r\n" \
>   | nc www.example.com www | sed 1q 
>   HTTP/1.0 200 OK
> 
> This behaviour strikes me as wrong (or at least sub-optimal) in the
> case of non-matching "Host:" headers.  The simplistic patch below
> changes things to return a 404 status if no matching server is found.
> 
> [If status code 400 (bad request) is preferred, "goto fail;"
> could be used.]
> 
> Justification:
> - This seems more correct, and is consistent with the "fail closed"
>   approach.

In which way can the current behaviour cause problems?

I dont think we should treat Host: headers as secrets, so there is no
information leakage or such a thing.

The downside of changing this is possible breakage in existing configs,
that should be avoided.

> - There is a net gain in functionality, as use of glob/patterns
>   wildcards can easily re-establish the current behaviour.  In
>   contrast, there's no way at present to disable the implicit
>   match-anything behaviour.

As jca@ shows the first host can be a dummy.

I kind of think that this is a documentation problem, we should docuemnt
this in the manpage and maybe example config:

diff --git etc/examples/httpd.conf etc/examples/httpd.conf
index fee8d607e90..67eb075eb3e 100644
--- etc/examples/httpd.conf
+++ etc/examples/httpd.conf
@@ -1,5 +1,11 @@
 # $OpenBSD: httpd.conf,v 1.20 2018/06/13 15:08:24 reyk Exp $
 
+# define a default server, to produce 404 responses for unknown hosts.
+server "default" {
+   listen on * port 80
+   root "/nonexistant"
+}
+
 server "example.com" {
listen on * port 80
location "/.well-known/acme-challenge/*" {
diff --git usr.sbin/httpd/httpd.conf.5 usr.sbin/httpd/httpd.conf.5
index 02b4442693b..45780cab78b 100644
--- usr.sbin/httpd/httpd.conf.5
+++ usr.sbin/httpd/httpd.conf.5
@@ -660,6 +660,12 @@ It is possible to set
 to default to use the httpd default timeout of 2 hours.
 .El
 .El
+.Pp
+The first
+.Ic server
+section defines an implicit default for all requests that are not served by 
other
+.Ic server
+declarations.
 .Sh TYPES
 Configure the supported media types.
 .Xr httpd 8



Re: pipex "idle-timeout" work with pppx(4).

2020-08-10 Thread Vitaliy Makkoveev
We are doing all wrong :)

We can just unlink pppx(4) related session from `pipex_session_list' if
it's time expired. But since this unlinked session is still exists in
pppx(4) layer we can access through pppx_get_closed() without any
search. We should only add flag to session which identifies it as
pppx(4) related.

I hope you like this idea.

 cut begin 
Static void
pipex_timer(void *ignored_arg)
{
struct pipex_session *session, *session_tmp;

timeout_add_sec(_timer_ch, pipex_prune);

NET_LOCK();
/* walk through */
LIST_FOREACH_SAFE(session, _session_list, session_list,
session_tmp) {
switch (session->state) {
case PIPEX_STATE_OPENED:
if (session->timeout_sec == 0)
continue;

session->stat.idle_time++;
if (session->stat.idle_time < session->timeout_sec)
continue;

if (session->pppx_session)
pipex_unlink_session(session);
else
pipex_notify_close_session(session);
break;
/* ... */
}

pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
{
struct pppx_if *pxi;

pxi = pppx_if_find(pxd, req->pdr_session_id, req->pdr_protocol);
if (pxi == NULL)
return (EINVAL);

memset(req, 0, sizeof(*req));
if (session->state == PIPEX_STATE_CLOSED) {
req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
pppx_if_destroy(pxi);   
}

return 0;
}

 cut end 


On Mon, Aug 10, 2020 at 04:30:27PM +0300, Vitaliy Makkoveev wrote:
> On Mon, Aug 10, 2020 at 03:12:02PM +0900, YASUOKA Masahiko wrote:
> > Hi,
> > 
> > Thank you for your review.
> > 
> > On Sun, 9 Aug 2020 20:03:50 +0300
> > Vitaliy Makkoveev  wrote:
> > > On Sun, Aug 09, 2020 at 06:20:13PM +0300, Vitaliy Makkoveev wrote:
> > >> You propose to unlink pppx(4) related session which reached timeout. I'm
> > >> ok with this direction. But I see no reason to rework _get_closed()
> > >> routines.
> > >> 
> > >> in pppac(4) case it's assumed what if session is not yet destroyed by
> > >> garbage collector, it will be destroyed while we performing PIPEXGCLOSED
> > >> command. We can make pppx(4) behavior the same and I propose to
> > >> pppx_get_closed() be like below. 
> > >> 
> > >> Also, nothing requires to modify pipex_get_closed(). 
> > >> 
> > >>  cut begin 
> > > 
> > > Sorry, I mean
> > > 
> > > pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> > > {
> > >   struct pppx_if  *pxi;
> > > 
> > >   memset(req, 0, sizeof(*req));
> > > 
> > >   while ((pxi = LIST_FIRST(>pxd_pxis))) {
> > >   if (pxi->pxi_session->state == session->state =
> > >   PIPEX_STATE_CLOSED) {
> > >   req->plr_ppp_id[req->plr_ppp_id_count++] =
> > >   pxi->pxi_session->ppp_id;
> > >   pppx_if_destroy(pxi);
> > >   }
> > >   }
> > > 
> > >   return 0;
> > > }
> > 
> > Yes, the diff doesn't seem to be completed but this way also will work.
> > 
> > Usually there is few CLOSED session even if there is a lot of session.
> > Also there is no CLOSED session if idle-timeout is not configured.  I
> > avoided that way because I think checking all sessions' state to find
> > such the few sessions is too expensive.
> > 
> > A way I am suggesting:
> > 
> > @@ -622,7 +625,7 @@ pipex_get_stat(struct pipex_session_stat
> >  
> >  Static int
> >  pipex_get_closed(struct pipex_session_list_req *req,
> > -struct pipex_iface_context *iface)
> > +int (*isowner)(void *, struct pipex_session *), void *ctx)
> >  {
> > struct pipex_session *session, *session_tmp;
> >  
> > @@ -630,7 +633,7 @@ pipex_get_closed(struct pipex_session_li
> > bzero(req, sizeof(*req));
> > LIST_FOREACH_SAFE(session, _close_wait_list, state_list,
> > session_tmp) {
> > -   if (session->pipex_iface != iface)
> > +   if (!isowner(ctx, session))
> > continue;
> > req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
> > LIST_REMOVE(session, state_list);
> > 
> > uses pipex_close_wait_list which contains only sessions which is timed
> > out.
> 
> You are right. pipex_get_closed() walks through `pipex_close_wait_list'
> which contains only CLOSE_WAIT sessions.
> 
> According to npppd(8) code we do PIPEXGCLOSED related walkthrough once
> per NPPPD_TIMER_TICK_IVAL seconds, which is defined as 4. Is this such
> performance impact?
> 
> Also who should destroy these sessions? It's assumed npppd(8) will
> destroy them by l2tp_ctrl_timeout() and pptp_ctrl_timeout()? Excuse me
> if I'm wrong, but who will destroy sessions in pppoe case?
> 
> > 
> > >> Also I 

Re: [Patch] Change httpd's handling of request "Host:" headers

2020-08-10 Thread Jeremie Courreges-Anglas
On Mon, Aug 10 2020, Ross L Richardson  wrote:
> Leo,
>
> On Mon, Aug 10, 2020 at 08:46:19AM +0200, Leo Unglaub wrote:
>> Hey,
>> i love your patch. The current behavour always bothered me because it caused
>> servers to display "wrong" sites as defaults for all requests missing the
>> Host header. I really like your patch and it works fine for me on my
>> servers.
>>[...]
>
> Thanks for testing.
>
> I forgot to mention that, by requiring an exact match, the patch should
> fix the port problem [see below] too.
>
> The problem:
>   $ printf "HEAD / HTTP/1.0\r\nHost: www.openbsd.org:22\r\n\r\n" \
>   | nc www.openbsd.org 80 | sed 1q
>   HTTP/1.0 200 OK

If you consider that a problem.  For example apache ignores any
client-provided port number.

  https://httpd.apache.org/docs/2.4/vhosts/details.html#namebased

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE



Re: [Patch] Change httpd's handling of request "Host:" headers

2020-08-10 Thread Jeremie Courreges-Anglas
On Sun, Aug 09 2020, Ross L Richardson  wrote:
> At present, if a request contains no "Host:" header [HTTP pre-1.1] or
> if the supplied header does not match any of the servers configured
> in httpd.conf, the request is directed to the first server.  This
> isn't documented, AFAICT.
>
> For example, if httpd.conf has just one server
>   server "www.example.com"
> then we currently get
>   $ printf "HEAD / HTTP/1.0\r\nHost: www.openbsd.org\r\n\r\n" \
>   | nc www.example.com www | sed 1q 
>   HTTP/1.0 200 OK
>
> This behaviour strikes me as wrong (or at least sub-optimal) in the
> case of non-matching "Host:" headers.  The simplistic patch below
> changes things to return a 404 status if no matching server is found.
>
> [If status code 400 (bad request) is preferred, "goto fail;"
> could be used.]
>
> Justification:
> - This seems more correct, and is consistent with the "fail closed"
>   approach.
> - There is a net gain in functionality, as use of glob/patterns
>   wildcards can easily re-establish the current behaviour.  In
>   contrast, there's no way at present to disable the implicit
>   match-anything behaviour.

The first server in my httpd config uses "root "/nonexistent".  This
results in proper 404 replies, so there is a way to disable the current
behavior.  I probably inferred this from the examples in the manpage.

My gut feeling is that the existing behavior is useful (you can
copy/paste the existing example in the manpage and serve files right
away under multiple host names) and I see no reason to break it.
Did you check whether this breaks existing mirrors?

> If this is adopted, it should be document in current.html
> A followup patch could merge this if statement with the one above it.
>
> Several other issues exist in "Host:" header handling.
>
> Ross
> --
>
> Index: server_http.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
> retrieving revision 1.140
> diff -u -p -r1.140 server_http.c
> --- server_http.c 3 Aug 2020 10:59:53 -   1.140
> +++ server_http.c 9 Aug 2020 04:37:08 -
> @@ -1200,7 +1200,7 @@ server_response(struct httpd *httpd, str
>   struct server_config*srv_conf = >srv_conf;
>   struct kv   *kv, key, *host;
>   struct str_find  sm;
> - int  portval = -1, ret;
> + int  hostmatch = 0, portval = -1, ret;
>   char*hostval, *query;
>   const char  *errstr = NULL;
>  
> @@ -1277,16 +1277,20 @@ server_response(struct httpd *httpd, str
>   /* Replace host configuration */
>   clt->clt_srv_conf = srv_conf;
>   srv_conf = NULL;
> + hostmatch = 1;
>   break;
>   }
>   }
>   }
>  
> - if (srv_conf != NULL) {
> + if (host == NULL) {
>   /* Use the actual server IP address */
>   if (server_http_host(>clt_srv_ss, hostname,
>   sizeof(hostname)) == NULL)
>   goto fail;
> + } else if (!hostmatch) {
> + server_abort_http(clt, 404, "not found");
> + return (-1);
>   } else {
>   /* Host header was valid and found */
>   if (strlcpy(hostname, host->kv_value, sizeof(hostname)) >=
>

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE



Re: explicit_bzero vs. alternatives

2020-08-10 Thread Janne Johansson
>
>
> OpenBSD has the explicit_bzero function to reliably (i.e. even if not
> observable in the C abstract machine) overwrite memory with zeroes.
> WG14 is currently considering adding similar functionality to C2X.
>
> Considered options include:
>
> * A function like explicit_bzero or memset_explicit, that overwrites the
> memory with a known value.
> * A function like memclear, that overwrites the memory in an
> implementation-defined manner, possibly using random data.
>
> Is there a rationale why OpenBSD went with their explicit_bzero design?
> Were alternatives considered and rejected?
>

Well, from what I remember it was put there in order to combat the fact
that compilers started to skip generating code for which they thought they
knew the thinking behind the source, and that if you bzero() some area and
don't read it you were not interested in the area and would not mind that
the compiler skipped the bzero altogether for a nice speedup.

Given the normal security stance of OpenBSD, not clearing out keys and
passwords for a perceived "speed increase" was considered bad and stupid,
and hence a function was created so that the compiler could not recognize
the name "bzero" and silently take away the key/pw-clearing parts of the
code.

So, given that one would mainly use this to wipe a key/pw as soon as it is
not used anymore AND that no later part of the code will be reading that
buffer again (which is why it was optimized away), there would be little
incentive to have memset_explicit to some other nonzero value which you
would not read ever, nor to have random data there you also never read
again.

If your program did care about the contents after a call to any of these
other *_explicit() versions then they would not be needed since the
compiler would not remove the original bzero()/memset() to begin with.

This, I would think, would be the reason for not having "fancy" versions of
choose-what-byte-to-wipe-the-key-with functions.

I don't see much of a threat in someone seeing my code writing zeros if
they could snoop the bus during the cleaning, nor a RAM version of the old
disk myth of  can read 1-5-10 old versions of your
data on a harddrive by reading slightly to the side of the track so you
must overwrite with random data 10 times to cover your tracks. Perhaps I am
not paranoid enough?

-- 
May the most significant bit of your life be positive.


Fwd: explicit_bzero vs. alternatives

2020-08-10 Thread Amit Kulkarni
moving to tech@

-- Forwarded message -
From: Philipp Klaus Krause 
Date: Mon, Aug 10, 2020 at 4:34 AM
Subject: explicit_bzero vs. alternatives
To: 


OpenBSD has the explicit_bzero function to reliably (i.e. even if not
observable in the C abstract machine) overwrite memory with zeroes.

WG14 is currently considering adding similar functionality to C2X.

Considered options include:

* A function like explicit_bzero or memset_explicit, that overwrites the
memory with a known value.
* A function like memclear, that overwrites the memory in an
implementation-defined manner, possibly using random data.

Is there a rationale why OpenBSD went with their explicit_bzero design?
Were alternatives considered and rejected?

Philipp



Re: pipex "idle-timeout" work with pppx(4).

2020-08-10 Thread Vitaliy Makkoveev
On Mon, Aug 10, 2020 at 03:12:02PM +0900, YASUOKA Masahiko wrote:
> Hi,
> 
> Thank you for your review.
> 
> On Sun, 9 Aug 2020 20:03:50 +0300
> Vitaliy Makkoveev  wrote:
> > On Sun, Aug 09, 2020 at 06:20:13PM +0300, Vitaliy Makkoveev wrote:
> >> You propose to unlink pppx(4) related session which reached timeout. I'm
> >> ok with this direction. But I see no reason to rework _get_closed()
> >> routines.
> >> 
> >> in pppac(4) case it's assumed what if session is not yet destroyed by
> >> garbage collector, it will be destroyed while we performing PIPEXGCLOSED
> >> command. We can make pppx(4) behavior the same and I propose to
> >> pppx_get_closed() be like below. 
> >> 
> >> Also, nothing requires to modify pipex_get_closed(). 
> >> 
> >>  cut begin 
> > 
> > Sorry, I mean
> > 
> > pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> > {
> > struct pppx_if  *pxi;
> > 
> > memset(req, 0, sizeof(*req));
> > 
> > while ((pxi = LIST_FIRST(>pxd_pxis))) {
> > if (pxi->pxi_session->state == session->state =
> > PIPEX_STATE_CLOSED) {
> > req->plr_ppp_id[req->plr_ppp_id_count++] =
> > pxi->pxi_session->ppp_id;
> > pppx_if_destroy(pxi);
> > }
> > }
> > 
> > return 0;
> > }
> 
> Yes, the diff doesn't seem to be completed but this way also will work.
> 
> Usually there is few CLOSED session even if there is a lot of session.
> Also there is no CLOSED session if idle-timeout is not configured.  I
> avoided that way because I think checking all sessions' state to find
> such the few sessions is too expensive.
> 
> A way I am suggesting:
> 
> @@ -622,7 +625,7 @@ pipex_get_stat(struct pipex_session_stat
>  
>  Static int
>  pipex_get_closed(struct pipex_session_list_req *req,
> -struct pipex_iface_context *iface)
> +int (*isowner)(void *, struct pipex_session *), void *ctx)
>  {
>   struct pipex_session *session, *session_tmp;
>  
> @@ -630,7 +633,7 @@ pipex_get_closed(struct pipex_session_li
>   bzero(req, sizeof(*req));
>   LIST_FOREACH_SAFE(session, _close_wait_list, state_list,
>   session_tmp) {
> - if (session->pipex_iface != iface)
> + if (!isowner(ctx, session))
>   continue;
>   req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
>   LIST_REMOVE(session, state_list);
> 
> uses pipex_close_wait_list which contains only sessions which is timed
> out.

You are right. pipex_get_closed() walks through `pipex_close_wait_list'
which contains only CLOSE_WAIT sessions.

According to npppd(8) code we do PIPEXGCLOSED related walkthrough once
per NPPPD_TIMER_TICK_IVAL seconds, which is defined as 4. Is this such
performance impact?

Also who should destroy these sessions? It's assumed npppd(8) will
destroy them by l2tp_ctrl_timeout() and pptp_ctrl_timeout()? Excuse me
if I'm wrong, but who will destroy sessions in pppoe case?

> 
> >> Also I have one inlined comment within your diff. 
> 
> >> > @@ -430,6 +425,7 @@ pipex_link_session(struct pipex_session 
> >> >  struct pipex_iface_context *iface)
> >> >  {
> >> >  struct pipex_hash_head *chain;
> >> > +struct ifnet *ifp;
> >> >  
> >> >  NET_ASSERT_LOCKED();
> >> >  
> >> > @@ -442,6 +438,11 @@ pipex_link_session(struct pipex_session 
> >> >  session->pipex_iface = iface;
> >> >  session->ifindex = iface->ifindex;
> >> >  
> >> > +ifp = if_get(iface->ifindex);
> >> > +if (ifp != NULL && ifp->if_flags & IFF_POINTOPOINT)
> >> > +session->is_p2p = 1;
> >> > +if_put(ifp);
> >> > +
> >> 
> >> I guess NULL `ifp' here exposes us a bug. I like to have assertion here.
> 
> ok, I agree here.
> 
> 
> The diff is updated.
> 
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 if_pppx.c
> --- sys/net/if_pppx.c 28 Jul 2020 09:53:36 -  1.98
> +++ sys/net/if_pppx.c 10 Aug 2020 06:09:52 -
> @@ -185,6 +185,7 @@ int   pppx_config_session(struct pppx_dev
>   struct pipex_session_config_req *);
>  int  pppx_get_stat(struct pppx_dev *,
>   struct pipex_session_stat_req *);
> +int  pppx_is_owner(void *, struct pipex_session *);
>  int  pppx_get_closed(struct pppx_dev *,
>   struct pipex_session_list_req *);
>  int  pppx_set_session_descr(struct pppx_dev *,
> @@ -645,14 +646,6 @@ pppx_add_session(struct pppx_dev *pxd, s
>   struct in_ifaddr *ia;
>   struct sockaddr_in ifaddr;
>  
> - /*
> -  * XXX: As long as `session' is allocated as part of a `pxi'
> -  *  it isn't possible to free it separately.  So disallow
> -  *  the timeout feature until this is fixed.
> -  */
> - if 

Re: [Patch] Change httpd's handling of request "Host:" headers

2020-08-10 Thread Ross L Richardson
Leo,

On Mon, Aug 10, 2020 at 08:46:19AM +0200, Leo Unglaub wrote:
> Hey,
> i love your patch. The current behavour always bothered me because it caused
> servers to display "wrong" sites as defaults for all requests missing the
> Host header. I really like your patch and it works fine for me on my
> servers.
>[...]

Thanks for testing.

I forgot to mention that, by requiring an exact match, the patch should
fix the port problem [see below] too.

The problem:
$ printf "HEAD / HTTP/1.0\r\nHost: www.openbsd.org:22\r\n\r\n" \
| nc www.openbsd.org 80 | sed 1q
HTTP/1.0 200 OK

Ross



Re: [Patch] Change httpd's handling of request "Host:" headers

2020-08-10 Thread Leo Unglaub

Hey,
i love your patch. The current behavour always bothered me because it 
caused servers to display "wrong" sites as defaults for all requests 
missing the Host header. I really like your patch and it works fine for 
me on my servers.


However, i am not an official dev, so i cannot give you an ok.
So this is just feedback!

Greetings
Leo

On 2020-08-09 12:07, Ross L Richardson wrote:


At present, if a request contains no "Host:" header [HTTP pre-1.1] or
if the supplied header does not match any of the servers configured
in httpd.conf, the request is directed to the first server.  This
isn't documented, AFAICT.

For example, if httpd.conf has just one server
server "www.example.com"
then we currently get
$ printf "HEAD / HTTP/1.0\r\nHost: www.openbsd.org\r\n\r\n" \
| nc www.example.com www | sed 1q
HTTP/1.0 200 OK

This behaviour strikes me as wrong (or at least sub-optimal) in the
case of non-matching "Host:" headers.  The simplistic patch below
changes things to return a 404 status if no matching server is found.

[If status code 400 (bad request) is preferred, "goto fail;"
could be used.]

Justification:
- This seems more correct, and is consistent with the "fail closed"
   approach.
- There is a net gain in functionality, as use of glob/patterns
   wildcards can easily re-establish the current behaviour.  In
   contrast, there's no way at present to disable the implicit
   match-anything behaviour.

If this is adopted, it should be document in current.html
A followup patch could merge this if statement with the one above it.

Several other issues exist in "Host:" header handling.

Ross
--

Index: server_http.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.140
diff -u -p -r1.140 server_http.c
--- server_http.c   3 Aug 2020 10:59:53 -   1.140
+++ server_http.c   9 Aug 2020 04:37:08 -
@@ -1200,7 +1200,7 @@ server_response(struct httpd *httpd, str
struct server_config*srv_conf = >srv_conf;
struct kv   *kv, key, *host;
struct str_find  sm;
-   int  portval = -1, ret;
+   int  hostmatch = 0, portval = -1, ret;
char*hostval, *query;
const char  *errstr = NULL;
  
@@ -1277,16 +1277,20 @@ server_response(struct httpd *httpd, str

/* Replace host configuration */
clt->clt_srv_conf = srv_conf;
srv_conf = NULL;
+   hostmatch = 1;
break;
}
}
}
  
-	if (srv_conf != NULL) {

+   if (host == NULL) {
/* Use the actual server IP address */
if (server_http_host(>clt_srv_ss, hostname,
sizeof(hostname)) == NULL)
goto fail;
+   } else if (!hostmatch) {
+   server_abort_http(clt, 404, "not found");
+   return (-1);
} else {
/* Host header was valid and found */
if (strlcpy(hostname, host->kv_value, sizeof(hostname)) >=





Re: pipex "idle-timeout" work with pppx(4).

2020-08-10 Thread YASUOKA Masahiko
Hi,

Thank you for your review.

On Sun, 9 Aug 2020 20:03:50 +0300
Vitaliy Makkoveev  wrote:
> On Sun, Aug 09, 2020 at 06:20:13PM +0300, Vitaliy Makkoveev wrote:
>> You propose to unlink pppx(4) related session which reached timeout. I'm
>> ok with this direction. But I see no reason to rework _get_closed()
>> routines.
>> 
>> in pppac(4) case it's assumed what if session is not yet destroyed by
>> garbage collector, it will be destroyed while we performing PIPEXGCLOSED
>> command. We can make pppx(4) behavior the same and I propose to
>> pppx_get_closed() be like below. 
>> 
>> Also, nothing requires to modify pipex_get_closed(). 
>> 
>>  cut begin 
> 
> Sorry, I mean
> 
> pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> {
>   struct pppx_if  *pxi;
> 
>   memset(req, 0, sizeof(*req));
> 
>   while ((pxi = LIST_FIRST(>pxd_pxis))) {
>   if (pxi->pxi_session->state == session->state =
>   PIPEX_STATE_CLOSED) {
>   req->plr_ppp_id[req->plr_ppp_id_count++] =
>   pxi->pxi_session->ppp_id;
>   pppx_if_destroy(pxi);
>   }
>   }
> 
>   return 0;
> }

Yes, the diff doesn't seem to be completed but this way also will work.

Usually there is few CLOSED session even if there is a lot of session.
Also there is no CLOSED session if idle-timeout is not configured.  I
avoided that way because I think checking all sessions' state to find
such the few sessions is too expensive.

A way I am suggesting:

@@ -622,7 +625,7 @@ pipex_get_stat(struct pipex_session_stat
 
 Static int
 pipex_get_closed(struct pipex_session_list_req *req,
-struct pipex_iface_context *iface)
+int (*isowner)(void *, struct pipex_session *), void *ctx)
 {
struct pipex_session *session, *session_tmp;
 
@@ -630,7 +633,7 @@ pipex_get_closed(struct pipex_session_li
bzero(req, sizeof(*req));
LIST_FOREACH_SAFE(session, _close_wait_list, state_list,
session_tmp) {
-   if (session->pipex_iface != iface)
+   if (!isowner(ctx, session))
continue;
req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
LIST_REMOVE(session, state_list);

uses pipex_close_wait_list which contains only sessions which is timed
out.

>> Also I have one inlined comment within your diff. 

>> > @@ -430,6 +425,7 @@ pipex_link_session(struct pipex_session 
>> >struct pipex_iface_context *iface)
>> >  {
>> >struct pipex_hash_head *chain;
>> > +  struct ifnet *ifp;
>> >  
>> >NET_ASSERT_LOCKED();
>> >  
>> > @@ -442,6 +438,11 @@ pipex_link_session(struct pipex_session 
>> >session->pipex_iface = iface;
>> >session->ifindex = iface->ifindex;
>> >  
>> > +  ifp = if_get(iface->ifindex);
>> > +  if (ifp != NULL && ifp->if_flags & IFF_POINTOPOINT)
>> > +  session->is_p2p = 1;
>> > +  if_put(ifp);
>> > +
>> 
>> I guess NULL `ifp' here exposes us a bug. I like to have assertion here.

ok, I agree here.


The diff is updated.

Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.98
diff -u -p -r1.98 if_pppx.c
--- sys/net/if_pppx.c   28 Jul 2020 09:53:36 -  1.98
+++ sys/net/if_pppx.c   10 Aug 2020 06:09:52 -
@@ -185,6 +185,7 @@ int pppx_config_session(struct pppx_dev
struct pipex_session_config_req *);
 intpppx_get_stat(struct pppx_dev *,
struct pipex_session_stat_req *);
+intpppx_is_owner(void *, struct pipex_session *);
 intpppx_get_closed(struct pppx_dev *,
struct pipex_session_list_req *);
 intpppx_set_session_descr(struct pppx_dev *,
@@ -645,14 +646,6 @@ pppx_add_session(struct pppx_dev *pxd, s
struct in_ifaddr *ia;
struct sockaddr_in ifaddr;
 
-   /*
-* XXX: As long as `session' is allocated as part of a `pxi'
-*  it isn't possible to free it separately.  So disallow
-*  the timeout feature until this is fixed.
-*/
-   if (req->pr_timeout_sec != 0)
-   return (EINVAL);
-
error = pipex_init_session(, req);
if (error)
return (error);
@@ -812,12 +805,22 @@ pppx_get_stat(struct pppx_dev *pxd, stru
 }
 
 int
-pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
+pppx_is_owner(void *ctx, struct pipex_session *session)
 {
-   /* XXX: Only opened sessions exist for pppx(4) */
-   memset(req, 0, sizeof(*req));
+   struct pppx_dev *pxd = ctx;
+   struct pppx_if *pxi;
 
-   return 0;
+   pxi = pppx_if_find(pxd, session->session_id, session->protocol);
+   if (pxi != NULL)
+   return (1);
+
+   return (0);
+}
+
+int
+pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
+{
+