security/sshlockout: use DIOCADDADDRS and remove "proc exec" from pledge.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

security/sshlockout: use DIOCADDADDRS and remove "proc exec" from pledge.

sunil+ports
Hi,

This diff replaces a system(3) call to insert an address into a pf
table with ioctl(DIOCADDADDRS) which allows removal of "proc exec"
from the pledge promises. Updated patch-sshlockout.c follows.

Please share suggestions/feedback.

Index: sshlockout.c
--- sshlockout.c.orig
+++ sshlockout.c
@@ -49,7 +49,14 @@
  */
 
 #include <sys/types.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
 #include <sys/time.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+#include <net/if.h>
+#include <net/pfvar.h>
+#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -81,11 +88,13 @@ struct args {
 #define MAXHIST 100
 #define SSHLIMIT 5 /* per hour */
 #define MAX_TABLE_NAME 20 /* PF table name limit */
+#define PF_DEVICE "/dev/pf"
 
 static iphist_t *hist_base;
 static iphist_t **hist_tail = &hist_base;
 static iphist_t *hist_hash[HSIZE];
 static int hist_count = 0;
+static int dev;
 
 static struct args args;
 
@@ -97,6 +106,8 @@ static void delete_iph(iphist_t *ip);
 static
 void
 block_ip(const char *ips) {
+ struct pfioc_table io;
+ struct pfr_addr addr;
  char buf[128];
  int r = 0;
 
@@ -113,7 +124,27 @@ block_ip(const char *ips) {
  }
 
  if (r > 0 && (int)strlen(buf) == r) {
- system(buf);
+ memset(&io, 0, sizeof(io));
+ strlcpy(io.pfrio_table.pfrt_name, args.arg1,
+    sizeof(io.pfrio_table.pfrt_name));
+ io.pfrio_esize = sizeof(addr);
+ io.pfrio_buffer = &addr;
+ io.pfrio_size = 1;
+
+ memset(&addr, 0, sizeof(addr));
+ if (inet_pton(AF_INET, ips, &addr.pfra_ip4addr) == 1) {
+ addr.pfra_af = AF_INET;
+ addr.pfra_net = 32;
+ } else if (inet_pton(AF_INET6, ips, &addr.pfra_ip6addr) == 1) {
+ addr.pfra_af = AF_INET6;
+ addr.pfra_net = 128;
+ } else {
+ syslog(LOG_ERR, "sshlockout: invalid ip: %s", ips);
+ return;
+ }
+
+ if (ioctl(dev, DIOCRADDADDRS, &io) == -1)
+ syslog(LOG_ERR, "sshlockout: ioctl(DIOCRADDADDRS): %m");
  }
  else {
  syslog(LOG_ERR, "sshlockout: invalid command");
@@ -198,6 +229,16 @@ main(int ac, char **av)
  syslog(LOG_ERR, "sshlockout starting up");
  freopen("/dev/null", "w", stdout);
  freopen("/dev/null", "w", stderr);
+
+ if ((dev = open(PF_DEVICE, O_RDWR)) == -1) {
+ syslog(LOG_ERR, "sshlockout: open(" PF_DEVICE "): %m");
+ return(1);
+ }
+
+ if (pledge("stdio", NULL) == -1) {
+ syslog(LOG_ERR, "sshlockout: pledge: %m");
+ return(1);
+ }
 
  while (fgets(buf, sizeof(buf), stdin) != NULL) {
  if (strstr(buf, "sshd") == NULL)

Reply | Threaded
Open this post in threaded view
|

Re: security/sshlockout: use DIOCADDADDRS and remove "proc exec" from pledge.

Jeremie Courreges-Anglas-5
On Sun, Nov 12 2017, [hidden email] wrote:
> Hi,

Hi Sunil,

> This diff replaces a system(3) call to insert an address into a pf
> table with ioctl(DIOCADDADDRS) which allows removal of "proc exec"
> from the pledge promises.

Interesting.  So DIOCRADDADDRS isn't restricted by pledge(2)?

> Updated patch-sshlockout.c follows.
>
> Please share suggestions/feedback.

The current sshlockout port isn't the most recent snapshot of the
upstream code.  sephe@dfly added ipfw table support in this commit:

  http://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/1e7ff9c9ed385f400d6e2624b42bc5020eeb379e

Since you're doing something similar for pf(4), I suggest that you
implement your code under a -pftbl switch.  Discussing this with
upstream would be good, especially if dfly can also use that code (it
seems they also have DIOCRADDADDRS).

Do you want me to update our port to the latest git HEAD?

> Index: sshlockout.c
> --- sshlockout.c.orig
> +++ sshlockout.c
> @@ -49,7 +49,14 @@
>   */
>  
>  #include <sys/types.h>
> +#include <sys/ioctl.h>
> +#include <sys/socket.h>
>  #include <sys/time.h>
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> +#include <net/if.h>
> +#include <net/pfvar.h>
> +#include <fcntl.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -81,11 +88,13 @@ struct args {
>  #define MAXHIST 100
>  #define SSHLIMIT 5 /* per hour */
>  #define MAX_TABLE_NAME 20 /* PF table name limit */
> +#define PF_DEVICE "/dev/pf"
>  
>  static iphist_t *hist_base;
>  static iphist_t **hist_tail = &hist_base;
>  static iphist_t *hist_hash[HSIZE];
>  static int hist_count = 0;
> +static int dev;
>  
>  static struct args args;
>  
> @@ -97,6 +106,8 @@ static void delete_iph(iphist_t *ip);
>  static
>  void
>  block_ip(const char *ips) {
> + struct pfioc_table io;
> + struct pfr_addr addr;
>   char buf[128];
>   int r = 0;
>  
> @@ -113,7 +124,27 @@ block_ip(const char *ips) {
>   }
>  
>   if (r > 0 && (int)strlen(buf) == r) {
> - system(buf);
> + memset(&io, 0, sizeof(io));
> + strlcpy(io.pfrio_table.pfrt_name, args.arg1,
> +    sizeof(io.pfrio_table.pfrt_name));
> + io.pfrio_esize = sizeof(addr);
> + io.pfrio_buffer = &addr;
> + io.pfrio_size = 1;
> +
> + memset(&addr, 0, sizeof(addr));
> + if (inet_pton(AF_INET, ips, &addr.pfra_ip4addr) == 1) {
> + addr.pfra_af = AF_INET;
> + addr.pfra_net = 32;
> + } else if (inet_pton(AF_INET6, ips, &addr.pfra_ip6addr) == 1) {
> + addr.pfra_af = AF_INET6;
> + addr.pfra_net = 128;
> + } else {
> + syslog(LOG_ERR, "sshlockout: invalid ip: %s", ips);
> + return;
> + }
> +
> + if (ioctl(dev, DIOCRADDADDRS, &io) == -1)
> + syslog(LOG_ERR, "sshlockout: ioctl(DIOCRADDADDRS): %m");
>   }
>   else {
>   syslog(LOG_ERR, "sshlockout: invalid command");
> @@ -198,6 +229,16 @@ main(int ac, char **av)
>   syslog(LOG_ERR, "sshlockout starting up");
>   freopen("/dev/null", "w", stdout);
>   freopen("/dev/null", "w", stderr);
> +
> + if ((dev = open(PF_DEVICE, O_RDWR)) == -1) {
> + syslog(LOG_ERR, "sshlockout: open(" PF_DEVICE "): %m");
> + return(1);
> + }
> +
> + if (pledge("stdio", NULL) == -1) {
> + syslog(LOG_ERR, "sshlockout: pledge: %m");
> + return(1);
> + }
>  
>   while (fgets(buf, sizeof(buf), stdin) != NULL) {
>   if (strstr(buf, "sshd") == NULL)
>
>

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

Reply | Threaded
Open this post in threaded view
|

Re: security/sshlockout: use DIOCADDADDRS and remove "proc exec" from pledge.

Stuart Henderson
On 2017/11/14 18:31, Jeremie Courreges-Anglas wrote:

> On Sun, Nov 12 2017, [hidden email] wrote:
> > Hi,
>
> Hi Sunil,
>
> > This diff replaces a system(3) call to insert an address into a pf
> > table with ioctl(DIOCADDADDRS) which allows removal of "proc exec"
> > from the pledge promises.
>
> Interesting.  So DIOCRADDADDRS isn't restricted by pledge(2)?

It looks like it would be restricted, it's not on the list of permitted
ioctls in the PLEDGE_PF section of kern_pledge.c. OTOH, DIOCRSETADDRS
and DIOCRCLRADDRS *are* permitted, so I don't think it would be
unreasonable to permit the remaining DIOCRxxxADDRS.

One reason for a port to call out to pfctl for PF-related operations
is to insulate it from kernel ABI changes (pfctl is more likely to be
up to date than packages after an update). I suppose at least for
sshlockout, it would fail open rather than closed if there were a
problem like this, so not likely to be a huge annoyance.

Reply | Threaded
Open this post in threaded view
|

Re: security/sshlockout: use DIOCADDADDRS and remove "proc exec" from pledge.

Theo de Raadt-2
> On 2017/11/14 18:31, Jeremie Courreges-Anglas wrote:
> > On Sun, Nov 12 2017, [hidden email] wrote:
> > > Hi,
> >
> > Hi Sunil,
> >
> > > This diff replaces a system(3) call to insert an address into a pf
> > > table with ioctl(DIOCADDADDRS) which allows removal of "proc exec"
> > > from the pledge promises.
> >
> > Interesting.  So DIOCRADDADDRS isn't restricted by pledge(2)?
>
> It looks like it would be restricted, it's not on the list of permitted
> ioctls in the PLEDGE_PF section of kern_pledge.c. OTOH, DIOCRSETADDRS
> and DIOCRCLRADDRS *are* permitted, so I don't think it would be
> unreasonable to permit the remaining DIOCRxxxADDRS.
>
> One reason for a port to call out to pfctl for PF-related operations
> is to insulate it from kernel ABI changes (pfctl is more likely to be
> up to date than packages after an update). I suppose at least for
> sshlockout, it would fail open rather than closed if there were a
> problem like this, so not likely to be a huge annoyance.

absolutely.  Don't do the ioctl by hand.

As to DIOCRADDADDRS and other ioctl, did you even test your diff
before sending it???

Reply | Threaded
Open this post in threaded view
|

Re: security/sshlockout: use DIOCADDADDRS and remove "proc exec" from pledge.

Sunil Nimmagadda-3
"Theo de Raadt" <[hidden email]> wrote:

> > On 2017/11/14 18:31, Jeremie Courreges-Anglas wrote:
> > > On Sun, Nov 12 2017, [hidden email] wrote:
> > > > Hi,
> > >
> > > Hi Sunil,
> > >
> > > > This diff replaces a system(3) call to insert an address into a pf
> > > > table with ioctl(DIOCADDADDRS) which allows removal of "proc exec"
> > > > from the pledge promises.
> > >
> > > Interesting.  So DIOCRADDADDRS isn't restricted by pledge(2)?
> >
> > It looks like it would be restricted, it's not on the list of permitted
> > ioctls in the PLEDGE_PF section of kern_pledge.c. OTOH, DIOCRSETADDRS
> > and DIOCRCLRADDRS *are* permitted, so I don't think it would be
> > unreasonable to permit the remaining DIOCRxxxADDRS.
> >
> > One reason for a port to call out to pfctl for PF-related operations
> > is to insulate it from kernel ABI changes (pfctl is more likely to be
> > up to date than packages after an update). I suppose at least for
> > sshlockout, it would fail open rather than closed if there were a
> > problem like this, so not likely to be a huge annoyance.
>
> absolutely.  Don't do the ioctl by hand.

As this program needs to run with elevated privileges, I was looking
for a way to reduce pledge promises. I didn't know ioctl(2) by hand
was discouraged.

>
> As to DIOCRADDADDRS and other ioctl, did you even test your diff
> before sending it???

Yes, I did but I did it wrong and I now realized my mistake.  I
forgot to make update-patches and was testing system("pfctl -tlockout
-Tadd x.x.x.x") assuming it was ioctl(2).

I installed a clean snapshot, ports tree and sshlockout with this
diff promptly crashes as ioctl(2) isn't allowed by pledge and syslogd
restarts sshlockout. Sorry for the noise. I will drop this diff.