Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[FRIGN]

On Wed, 28 Sep 2016 22:04:05 +0200
Klemens Nanni  wrote:

Hey Klemens,

> It's implicitly blocked by capturing the keys being pressed and
> executing optional steps such as shutting down upon input.

what if somebody remaps this functionality? This is possible with Xkb
sadly.

> Fair point, I shall fix this.

You cannot fix this at this level. As Markus already pointed out, you
have to carefully handle the options not to "destroy" the layout at the
end. Also, what if some automated script changes the options regularly?
You cannot cache the options reliably.

Cheers

FRIGN

-- 
FRIGN 

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Klemens Nanni]

On Wed, Sep 28, 2016 at 09:48:25PM +0200, FRIGN wrote:

Setting `DontVTSwitch' in xorg.conf(5) disables this feature
completely whereas chjj's fork (which mine is based on) blocks it in
slock only, which is imho a much saner approach since there are many
legitimate reasons to use multiple virtual terminals.


Can you point me to the piece of code that disables VT-switching in
your fork? I couldn't find it.

It's implicitly blocked by capturing the keys being pressed and
executing optional steps such as shutting down upon input.


Same story for `DontZap': I like quickly killing X with Ctrl+Alt+BS
while this should obviously be forbidden on a locked screen.


What you do is call
system("doas setxkbmap -option &");
which disables Ctrl+Alt+Backspace for the entire session. So you can
only kill your X server until you have locked your screen once. It
won't work afterwards, which sucks and is unpredictable.

Fair point, I shall fix this.


signature.asc
Description: PGP signature

[hackers] [slock] Add a section on security considerations || FRIGN

[git]

commit bd069b08c5fe4fea3c78f6991a849b19ed40cbe0
Author: FRIGN 
AuthorDate: Wed Sep 28 20:20:51 2016 +0200
Commit: Markus Teich 
CommitDate: Wed Sep 28 22:01:58 2016 +0200

Add a section on security considerations

The section on security considerations sheds some light on the problems 
that we
can't solve within slock but which the user has to solve in his X 
configuration.

diff --git a/slock.1 b/slock.1
index 2b2b7c8..82cdcd6 100644
--- a/slock.1
+++ b/slock.1
@@ -17,6 +17,18 @@ is executed after the screen has been locked.
 .It Fl v
 Print version information to stdout and exit.
 .El
+.Sh SECURITY CONSIDERATIONS
+To make sure a locked screen can not be bypassed by switching VTs
+or killing the X server with Ctrl+Alt+Backspace, it is recommended
+to disable both in
+.Xr xorg.conf 5
+for maximum security:
+.Bd -literal -offset left
+Section "ServerFlags"
+   Option "DontVTSwitch" "True"
+   Option "DontZap"  "True"
+EndSection
+.Ed
 .Sh EXAMPLES
 $
 .Nm

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Markus Teich]

FRIGN wrote:
> What you do is call
>   system("doas setxkbmap -option &");
> which disables Ctrl+Alt+Backspace for the entire session. So you can
> only kill your X server until you have locked your screen once. It
> won't work afterwards, which sucks and is unpredictable.

Heyho,

as an additional disadvantage this disables all options, even those which the
user might need to enter his password.

--Markus

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Markus Teich]

Klemens Nanni wrote:
> Setting `DontVTSwitch' in xorg.conf(5) disables this feature completely
> whereas chjj's fork (which mine is based on) blocks it in slock only, which is
> imho a much saner approach since there are many legitimate reasons to use
> multiple virtual terminals.
> 
> Same story for `DontZap': I like quickly killing X with Ctrl+Alt+BS while this
> should obviously be forbidden on a locked screen.

Heyho Klemens,

Just put a wrapper script around it, if you don't want to enforce it as root.
That's what I meant by 'flexibility' and it is mentioned in the manpage after
the patch to empower you to choose to do it if you want. ;)

--Markus

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[FRIGN]

On Wed, 28 Sep 2016 21:41:36 +0200
Klemens Nanni  wrote:

Hey Klemens,

> I removed media upload and SMS support since those features can easily
> be added using a small wrapper script.

I don't see the gain anyway with that but to each his own. If somebody
tried to access my computer, it gives the red color, which is
sufficient. All the cruft introduced with these changes just makes
slock more insecure.

> Setting `DontVTSwitch' in xorg.conf(5) disables this feature
> completely whereas chjj's fork (which mine is based on) blocks it in
> slock only, which is imho a much saner approach since there are many
> legitimate reasons to use multiple virtual terminals.

Can you point me to the piece of code that disables VT-switching in
your fork? I couldn't find it.

> Same story for `DontZap': I like quickly killing X with Ctrl+Alt+BS
> while this should obviously be forbidden on a locked screen.

What you do is call
system("doas setxkbmap -option &");
which disables Ctrl+Alt+Backspace for the entire session. So you can
only kill your X server until you have locked your screen once. It
won't work afterwards, which sucks and is unpredictable.

Cheers

FRIGN

-- 
FRIGN 

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Klemens Nanni]

On Wed, Sep 28, 2016 at 09:09:24PM +0200, FRIGN wrote:

I know this fork, and with the changes presented in this patch, slock
is just as secure as his version.
The difference is that he for instance implemented ways to upload
webcam images to imgur, send SMS's and auto-shutdown when the user
tries to switch VT's.

I removed media upload and SMS support since those features can easily
be added using a small wrapper script.


I think these changes are not necessary. If somebody tries to change
VT's, so be it! Especially because the shutdown sequence can open other
attack surfaces, which he also took care of mostly, by disallowing the
use of Sysrq in the shutdown sequence. In my opinion, with a strong
password and setting the configs as in the manpage, slock is damn
secure. It honestly took me a few days to analyze the "paranoid" slock
fork to find out that what I did was sufficient.

Setting `DontVTSwitch' in xorg.conf(5) disables this feature completely
whereas chjj's fork (which mine is based on) blocks it in slock only,
which is imho a much saner approach since there are many legitimate
reasons to use multiple virtual terminals.

Same story for `DontZap': I like quickly killing X with Ctrl+Alt+BS
while this should obviously be forbidden on a locked screen.

Best regards,
kl3


signature.asc
Description: PGP signature

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Markus Teich]

FRIGN wrote:
> > I don't think it is that obvious. Have a look at the discussion starting
> > from the slock-1.3 announcement on February 12th this year again. Since the
> > example does not work any more, changing it to `slock sudo s2ram` and adding
> > a note about the needed line in the sudo config so s2ram can be run without
> > a password would be better.
> 
> the problem doesn't end there. Also, s2ram is Linux specific and in 99% of the
> cases you run unprivileged after-lock commands.  To be honest, I expect any
> half-decent user how to set up doas or sudo.

Heyho,

it's called *example* for a reason. ;)

I think it serves two purposes very well:

1.) Hint the user that he can put his computer to sleep mode. Of course it's
linux specific, but I estimate the probability of a non-linux user being able to
adapt that higher than the probability of any user coming up with this use case
at all.

2.) Show that the post-lock command is not run as root by default. Obviously
sudo is just one way of regaining root for the post-lock command, but it's the
most common and known one and therefore fits well for an *example*. The example
sudo config line is not necessary however, so feel free to leave it out.

--Markus

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[FRIGN]

On Wed, 28 Sep 2016 21:17:23 +0200
Markus Teich  wrote:

Hey Markus,

> I don't think it is that obvious. Have a look at the discussion
> starting from the slock-1.3 announcement on February 12th this year
> again. Since the example does not work any more, changing it to
> `slock sudo s2ram` and adding a note about the needed line in the
> sudo config so s2ram can be run without a password would be better.

the problem doesn't end there. Also, s2ram is Linux specific and in 99%
of the cases you run unprivileged after-lock commands.
To be honest, I expect any half-decent user how to set up doas or sudo.

> The other block looks good and is more simple and flexible than
> changing the xkb config ourselves from slock.c.

Yes, the problem really is that if we did the xkb changes within
slock.c, we somehow would have to cache the option string. If for any
reason it is changed while slock is running (automation, whatever),
and we uncache it, this can lead to strange behaviour.

Cheers

FRIGN

-- 
FRIGN 

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Markus Teich]

FRIGN wrote:
> The given example does not work and the usage is so obvious that an example
> probably is not necessary here anyway.

Heyho,

I don't think it is that obvious. Have a look at the discussion starting from
the slock-1.3 announcement on February 12th this year again. Since the example
does not work any more, changing it to `slock sudo s2ram` and adding a note
about the needed line in the sudo config so s2ram can be run without a password
would be better.

The other block looks good and is more simple and flexible than changing the xkb
config ourselves from slock.c.

--Markus

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Ali H. Fardan]

PS: I think this is where the code originated: 
https://github.com/chjj/slock

Re: [hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[Ali H. Fardan]

I suggest you take a look at this:
https://notabug.org/kl3/slock

it was used to be called "slock for the absolute paranoid", but this 
dude

wanted to go further with it and make it fit his taste, but there are
some security stuff he did there, check it out.

--
Raiz

On 2016-09-28 21:33, FRIGN wrote:

Hello fellow hackers,

the question has been floating around for quite some time on the
internet, but I think it is a good place to answer it in the manual of
our screen locker. Is slock really secure and if not, how can I harden
it so that nobody can access my machine?

There are two ways one can possibly circumvent a locked X screen (not
including security holes in the Kernel)

1) switch to a different VT that is logged in. Then there, proceed to
   kill slock and switch back the now unlocked VT.
2) kill the X server with Ctrl+Alt+Backspace (if enabled). If no login
   manager is used, this yields an open shell. All work within the X
   session is usually lost, but the attacker still has access to the
   user data.

Sysrq can be used to kill all running processes, but this also logs out
the user and thus is no problem. I did not add it here because if
somebody wants to "pwn" the user he can just unplug the computer or
take out the battery to destroy all the work.

You can disable VT switching and Ctrl+Alt+Backspace (this also
overrides the local Xkb settings and is thus foolproof) for sure by
setting two options in xorg.conf. See the patch for details on the
instructions.

Cheers

FRIGN

[hackers] [PATCH] [slock] Remove faulty example and add a section on security considerations

[FRIGN]

Hello fellow hackers,

the question has been floating around for quite some time on the
internet, but I think it is a good place to answer it in the manual of
our screen locker. Is slock really secure and if not, how can I harden
it so that nobody can access my machine?

There are two ways one can possibly circumvent a locked X screen (not
including security holes in the Kernel)

1) switch to a different VT that is logged in. Then there, proceed to
   kill slock and switch back the now unlocked VT.
2) kill the X server with Ctrl+Alt+Backspace (if enabled). If no login
   manager is used, this yields an open shell. All work within the X
   session is usually lost, but the attacker still has access to the
   user data.

Sysrq can be used to kill all running processes, but this also logs out
the user and thus is no problem. I did not add it here because if
somebody wants to "pwn" the user he can just unplug the computer or
take out the battery to destroy all the work.

You can disable VT switching and Ctrl+Alt+Backspace (this also
overrides the local Xkb settings and is thus foolproof) for sure by
setting two options in xorg.conf. See the patch for details on the
instructions.

Cheers

FRIGN

-- 
FRIGN 
>From 2e363c4dfc98153f8067df27673dda9047ab9227 Mon Sep 17 00:00:00 2001
From: FRIGN 
Date: Wed, 28 Sep 2016 20:20:51 +0200
Subject: [PATCH] Remove faulty example and add a section on security
 considerations

The given example does not work and the usage is so obvious that an
example probably is not necessary here anyway.

The section on security considerations sheds some light on the
problems that we can't solve within slock but which the user has to
solve in his X configuration.
---
 slock.1 | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/slock.1 b/slock.1
index 2b2b7c8..6c1e8bd 100644
--- a/slock.1
+++ b/slock.1
@@ -17,10 +17,18 @@ is executed after the screen has been locked.
 .It Fl v
 Print version information to stdout and exit.
 .El
-.Sh EXAMPLES
-$
-.Nm
-/usr/sbin/s2ram
+.Sh SECURITY CONSIDERATIONS
+To make sure a locked screen can not be bypassed by switching VTs
+or killing the X server with Ctrl+Alt+Backspace, it is recommended
+to disable both in
+.Xr xorg.conf 5
+for maximum security:
+.Bd -literal -offset left
+Section "ServerFlags"
+	Option "DontVTSwitch" "True"
+	Option "DontZap"  "True"
+EndSection
+.Ed
 .Sh CUSTOMIZATION
 .Nm
 can be customized by creating a custom config.h from config.def.h and
-- 
2.7.3

[hackers] [scc] [cc1] Fix size/offset calculation for structs || Roberto E. Vargas Caballero

[git]

commit 9761a80a98bd2f59d922f83862f4faa7a4389861
Author: Roberto E. Vargas Caballero 
AuthorDate: Wed Sep 28 12:28:00 2016 +0200
Commit: Roberto E. Vargas Caballero 
CommitDate: Wed Sep 28 12:28:00 2016 +0200

[cc1] Fix size/offset calculation for structs

The code was adding when it should only assign. With the previous
code we were duplicating the offset for every field with aligment
different of 0.

diff --git a/cc1/types.c b/cc1/types.c
index 44deac6..d49b3e5 100644
--- a/cc1/types.c
+++ b/cc1/types.c
@@ -210,7 +210,7 @@ typesize(Type *tp)
align = a;
if (tp->op == STRUCT) {
if (--a != 0)
-   size += (size + a) & ~a;
+   size = (size + a) & ~a;
size += aux->size;
offset = size;
} else {

[hackers] [scc] [cc1] Simplify expression in types.c || Roberto E. Vargas Caballero

[git]

commit bea83b8b7641be12d18d3f69f658261459de7fc4
Author: Roberto E. Vargas Caballero 
AuthorDate: Wed Sep 28 12:34:53 2016 +0200
Commit: Roberto E. Vargas Caballero 
CommitDate: Wed Sep 28 12:34:53 2016 +0200

[cc1] Simplify expression in types.c

diff --git a/cc1/types.c b/cc1/types.c
index d49b3e5..170e4a8 100644
--- a/cc1/types.c
+++ b/cc1/types.c
@@ -214,7 +214,7 @@ typesize(Type *tp)
size += aux->size;
offset = size;
} else {
-   if ((*sp)->type->size > size)
+   if (aux->size > size)
size = aux->size;
}
}

[hackers] [scc] [tests] Add a list of tests for scc || Roberto E. Vargas Caballero

[git]

commit fb1c921d30051447b2de4fb500de943e270756fe
Author: Roberto E. Vargas Caballero 
AuthorDate: Wed Sep 28 11:56:19 2016 +0200
Commit: Roberto E. Vargas Caballero 
CommitDate: Wed Sep 28 11:56:19 2016 +0200

[tests] Add a list of tests for scc

Not all the tests of the ac test suite are suitable for
scc, because some of the implement features of c11, which
is not supported by scc. The idea is to create different
lists and let to the use the decision of what list to run.

diff --git a/tests/scc-tests.lst b/tests/scc-tests.lst
new file mode 100644
index 000..1782198
--- /dev/null
+++ b/tests/scc-tests.lst
@@ -0,0 +1,78 @@
+0001-sanity.c
+0002-expr.c
+0003-local.c
+0004-pointer.c
+0005-ifstmt.c
+0006-whilestmt.c
+0007-forstmt.c
+0008-dowhilestmt.c
+0009-expr.c
+0010-goto.c
+0011-assign.c
+0012-expr.c
+0013-addridx.c
+0014-assignidx.c
+0015-localarray.c
+0016-addrarray.c
+0017-struct.c
+0018-structptr.c
+0019-selfrefstruct.c
+0020-ptrptr.c
+0021-intfunc.c
+0022-typedef.c
+0023-global.c
+0024-typedefstruct.c
+0025-string.c
+0026-implicitret.c
+0027-charval.c
+0028-bor.c
+0029-band.c
+0030-bxor.c
+0031-relop.c
+0032-indec.c
+0033-ptrindec.c
+0034-logandor.c
+0035-breakcont.c
+0036-notneg.c
+0037-assignop.c
+0038-ptradd.c
+0039-sizeof.c
+0040-cast.c
+0041-queen.c
+0042-prime.c
+0043-union.c
+0044-struct.c
+0045-struct.c
+0046-inits.c
+0048-inits.c
+0049-inits.c
+0050-inits.c
+0052-switch.c
+0053-struct.c
+0054-struct.c
+0055-enum.c
+0056-enum.c
+0057-duff.c
+0058-bug.c
+0059-multistring.c
+0060-charlit.c
+0061-comments.c
+0062-include.c
+0063-define.c
+0064-sysinclude.c
+0065-ifdef.c
+0066-cppelse.c
+0067-define.c
+0068-funclikemacro.c
+0069-funclikemacro.c
+0070-cppif.c
+0071-cppelif.c
+0072-cppelif.c
+0073-ifndef.c
+0074-undef.c
+0075-ptraddasn.c
+0076-ptrsubasn.c
+0077-defined.c
+0078-dirifexpr.c
+0079-cond.c
+0080-arrays.c

[hackers] [scc] [cc2-qbe] Jump at the end in switches || Roberto E. Vargas Caballero

[git]

commit 95ac9724aaa7e8aecbfc0b354ab30a096c08ebbd
Author: Roberto E. Vargas Caballero 
AuthorDate: Wed Sep 28 11:49:57 2016 +0200
Commit: Roberto E. Vargas Caballero 
CommitDate: Wed Sep 28 11:49:57 2016 +0200

[cc2-qbe] Jump at the end in switches

When we arrive to the end of a switch if-else-if chain
we have to jump to the default label. If we do not have
default case it means that we have to jump to the end of
the switch, but we always have to jump.

diff --git a/cc2/arch/qbe/cgen.c b/cc2/arch/qbe/cgen.c
index 1c7b8fb..39ee9ec 100644
--- a/cc2/arch/qbe/cgen.c
+++ b/cc2/arch/qbe/cgen.c
@@ -428,12 +428,12 @@ swtch_if(Node *idx)
 
switch (np->op) {
case OESWITCH:
-   if (deflabel) {
-   aux1.op = OJMP;
-   aux1.label = NULL;
-   aux1.u.sym = deflabel;
-   cgen();
-   }
+   if (!deflabel)
+   deflabel = np->u.sym;
+   aux1.op = OJMP;
+   aux1.label = NULL;
+   aux1.u.sym = deflabel;
+   cgen();
return;
case OCASE:
aux1 = *np;

[hackers] [scc] [cc1] Fix redeclaration of tags || Roberto E. Vargas Caballero

[git]

commit 44123dfb36d1c0ba6b428d89a870679f4f7ff86e
Author: Roberto E. Vargas Caballero 
AuthorDate: Wed Sep 28 11:32:54 2016 +0200
Commit: Roberto E. Vargas Caballero 
CommitDate: Wed Sep 28 11:32:54 2016 +0200

[cc1] Fix redeclaration of tags

A tag can be redeclared if the context of both declarations is different.

diff --git a/cc1/decl.c b/cc1/decl.c
index 20df8fd..05677ab 100644
--- a/cc1/decl.c
+++ b/cc1/decl.c
@@ -519,7 +519,7 @@ structdcl(void)
return tp;
}
 
-   if (tp->prop & TDEFINED)
+   if (tp->prop & TDEFINED && sym->ctx == curctx)
error("redefinition of struct/union '%s'", sym->name);
tp->prop |= TDEFINED;