Re: [hackers] [surf][PATCH] Don't call setstyle with an empty path
On Thu, May 05, 2022 at 09:11:08AM +0200, Antonio Quartulli wrote: > On 05/05/2022 05:38, Nick Hanley wrote: > > On Wed, May 04, 2022 at 09:49:33AM +0200, Antonio Quartulli wrote: > >> On 04/05/2022 00:18, Nick Hanley wrote: > >>> On Tue, May 03, 2022 at 02:45:05PM -0400, fo...@dnmx.org wrote: > >>>> Just wondering - why not just do > >>>> if (*getstyle(geturi(c))) > >>>> setstyle(c, file); > >>> > >>> We need to pass the file path returned by getstyle to setstyle. > >> > >> An alternative would be to keep the original nested invocation and add a > >> NULL check inside setstyle(). This would avoid adding an extra variable > >> 'file'. > > > > Is there a reason to avoid declaring a variable? > > Well, the variable was not there, so the question is: is there a reason > to add it? :-D > IMHO the code is easier without it (as the variable is not used anywhere > else) I think checking that getstyle returned a valid path is clearer than putting a NULL check in setstyle. It's the caller's responsibility to pass valid arguments, and there's no reason to call setstyle with NULL otherwise. P.S. Apologies for the delay. Mail server gremlins ate my reply.
Re: [hackers] [surf][PATCH] Don't call setstyle with an empty path
On Wed, May 04, 2022 at 09:49:33AM +0200, Antonio Quartulli wrote: > On 04/05/2022 00:18, Nick Hanley wrote: > > On Tue, May 03, 2022 at 02:45:05PM -0400, fo...@dnmx.org wrote: > >> Just wondering - why not just do > >> if (*getstyle(geturi(c))) > >>setstyle(c, file); > > > > We need to pass the file path returned by getstyle to setstyle. > > An alternative would be to keep the original nested invocation and add a > NULL check inside setstyle(). This would avoid adding an extra variable > 'file'. Is there a reason to avoid declaring a variable?
Re: [hackers] [surf][PATCH] Don't call setstyle with an empty path
On Tue, May 03, 2022 at 02:45:05PM -0400, fo...@dnmx.org wrote: > Just wondering - why not just do > if (*getstyle(geturi(c))) > setstyle(c, file); We need to pass the file path returned by getstyle to setstyle.
[hackers] [surf][PATCH] Don't call setstyle with an empty path
There is no need to call setstyle if there is no style to be set. Fixes
spurious warnings about unreadable style files.
---
surf.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/surf.c b/surf.c
index 010e5e2..2192025 100644
--- a/surf.c
+++ b/surf.c
@@ -862,8 +862,11 @@ setparameter(Client *c, int refresh, ParamName p, const
Arg *a)
case Style:
webkit_user_content_manager_remove_all_style_sheets(
webkit_web_view_get_user_content_manager(c->view));
- if (a->i)
- setstyle(c, getstyle(geturi(c)));
+ if (a->i) {
+ const char *file = getstyle(geturi(c));
+ if (file)
+ setstyle(c, file);
+ }
refresh = 0;
break;
case WebGL:
@@ -936,7 +939,7 @@ getstyle(const char *uri)
return styles[i].file;
}
- return "";
+ return NULL;
}
void
--
2.35.1
Re: [hackers] [quark] Thoughts on CGI and authentication?
Quoth José Miguel Sánchez García: > Thanks for suggesting basic! I wasn't sure about it, as it's pretty > insecure nowadays. But I acknowledge that, for quark's use cases, it > is perfectly reasonable. I don't think it's insecure presuming the HTTP is being served behind some TLS connection. And if you're doing authentication you want that anyway. I haven't particularly thought it through, though, maybe there's something dangerous about it. I mean, lack of browser support for a straightforward "log out" function sucks, but hey, it's the web, of course it's broken. The filesystem based thing sounds odd to me, personally - I think it's common for websites to have a quite different set of users to those that exist on the server operating system. But I think setting it in config.h is also a bad idea, as one of the nice design things about quark is the ability to run it straight from the command line, and needing to recompile to redo authentication would detract from that. Maybe a simple authentication file with usernamepassword one per line, which is passed to a flag, would be good? If you want a system with different files accessible to different users, though, then reusing filesystem permissions is the only non-intrusive way I can imagine. Just some early morning thoughts. I look forward to Anselm replying and saying that authentication is out of scope for quark, keeping us all honest ;) Nick
Re: [hackers] [dwm] [patch] tatami layout
Quoth Hiltjo Posthuma: > On Thu, Sep 24, 2020 at 02:00:47PM +0530, Sarthak Shah wrote: > > This patch adds a tatami arrangement to dwm, accessible through modkey+y > > Please submit this to the wiki as it's not an upstream patch and do not submit > non-upstream patches to hackers@. > > https://suckless.org/community/ In addition it would be nice if you could include a diagram demonstrating what the layout looks like; see how this does it as a nice example: http://dwm.suckless.org/patches/fibonacci/
Re: [hackers] [dmenu][PATCH] Replace dmenu_run shell with executed application
Quoth Hiltjo Posthuma: > On Sat, Feb 09, 2019 at 01:48:34PM +0100, Jan Bessai wrote: > > On Sun, Dec 30, 2018 at 02:59:13PM +0100, Jan Bessai wrote: > > > Thanks! I've attached the updated patch below. > > > > Sorry if I'm breaching any rules, but any update on accepting/rejecting > > the patch? > > > > Rejected Ignore if you're too busy, but why is this considered bad practise? Is there some case of possible shell escaping or something I'm failing to see? I just ask for my own education.
Re: [hackers] [quark] Initial commit of quark rewrite || FRIGN
Hi FRIGN,
I don't have time to look at this closely now, but one little thing
that jumped out at me:
> +static const struct {
> + char *ext;
> + char *type;
> +} mimes[] = {
> ...
Why are you defining the struct in config.def.h? Shouldn't it be
defined in quark.c, and then just populated in config.def.h. That is
what other projects do, and it makes more sense, as the code in
quark.c depends on the struct being defined a certain way.
Nick
[hackers] [surf] [PATCH] Revert "Get rid of getkbdevice"
The webkit2 branch of surf recently started using a function that
requires GTK 3.20+, which was only released on 2016-03-21. I'm
running Debian Jessie, which means it would be rather non-trivial to
install such a version, so I'd prefer that the old method is used
instead.
This patch reverts that commit, and surf is now buildable again on
my system with GTK 3.14. Whether you want to take this patch
upstream, I don't know, but it's here if you do.
Nick
>From 908b464eb8a8871125a52e565cd4d02b6cd8f54c Mon Sep 17 00:00:00 2001
From: Nick White <g...@njw.name>
Date: Thu, 18 Aug 2016 10:14:56 +0100
Subject: [PATCH] Revert "Get rid of getkbdevice"
This reverts commit f9714ab838e362a74e02916317cf22ef0ebdcdb6.
That commit bumped the minimum version requirement of GTK to 3.20,
which was released on 2016-03-21.
Conflicts:
surf.c
---
surf.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/surf.c b/surf.c
index 29b9ede..e8ccaa6 100644
--- a/surf.c
+++ b/surf.c
@@ -174,6 +174,7 @@ static void destroyclient(Client *c);
static void cleanup(void);
/* GTK/WebKit */
+static GdkDevice *getkbdevice(void);
static WebKitWebView *newview(Client *c, WebKitWebView *rv);
static GtkWidget *createview(WebKitWebView *v, WebKitNavigationAction *a,
Client *c);
@@ -287,7 +288,7 @@ setup(void)
scriptfile = buildfile(scriptfile);
cachedir = buildpath(cachedir);
- gdkkb = gdk_seat_get_keyboard(gdk_display_get_default_seat(gdpy));
+ gdkkb = getkbdevice();
if (!stylefile) {
styledir = buildpath(styledir);
@@ -902,6 +903,22 @@ cleanup(void)
g_free(cachedir);
}
+static GdkDevice *
+getkbdevice(void)
+{
+ GList *l, *gdl = gdk_device_manager_list_devices(
+ gdk_display_get_device_manager(gdk_display_get_default()),
+ GDK_DEVICE_TYPE_MASTER);
+ GdkDevice *gd = NULL;
+
+ for (l = gdl; l != NULL; l = l->next)
+ if (gdk_device_get_source(l->data) == GDK_SOURCE_KEYBOARD)
+ gd = l->data;
+
+ g_list_free(gdl);
+ return gd;
+}
+
WebKitWebView *
newview(Client *c, WebKitWebView *rv)
{
--
2.1.4
Re: [hackers] [dmenu] [PATCHES] Improve code readability
Quoth Klemens Nanni: > be thrown away as well, but how is turning this lengthy and empty-bodied > for loop into a much clearer while loop *not* an improvement in readability? I found that to be an improvement in readability too, fwiw.
[hackers]
unsubscribe
Re: [hackers] [st] Patch to workaround missing st terminfo on remote SSH.
Quoth Roberto E. Vargas Caballero: I think a better aproach is to define an alias like this: alias ssh=TERM='TERM=xterm ssh' Syntax like that is one reason that I prefer one or two line shell scripts to aliases. Good idea, though.
Re: [hackers] [st][PATCH] Add total compability to the Makefiles
The comment line for this is wrong; it's definitely not for st. Did this come from a commit hook? If so, it needs to be fixed.
Re: [hackers] [sbase] tar: fix bug when extracting a tarball created using '.' as base path
Quoth Dimitris Papastamos:
Would you mind posting any dependent patches as well? I think I've done
something stupid and lost the thread.
Sure, I've attached this patch and the previous one.
I have one more patch planned, to refuse to create files that use
created symlinks in their path, but I haven't written that yet, and
these are still welcome improvements without it.
Nick
From b5acf1e9254080c2f283c623f59e412cdb29939a Mon Sep 17 00:00:00 2001
From: Nick White g...@njw.name
Date: Mon, 27 Apr 2015 12:25:44 +0100
Subject: [PATCH 1/2] Strip dangerous path traversal stuff from paths
Specifically, this removes '../' from anywhere in a path, and any
number of '/' at the start of a path
---
tar.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/tar.c b/tar.c
index b1f3b27..0bd3fcf 100644
--- a/tar.c
+++ b/tar.c
@@ -359,6 +359,27 @@ sanitize(struct header *h)
}
static void
+sanitizepath(char *p)
+{
+ size_t l;
+ char *s;
+
+ /* Strip leading '/' characters */
+ while(*p == '/') {
+ l = strlen(p);
+ memmove(p, p+1, l - 1);
+ *(p + l - 1) = '\0';
+ }
+
+ /* Strip '../' from anywhere */
+ while((s = strstr(p, ../)) != NULL) {
+ l = strlen(s);
+ memmove(s, s + 3, l - 3);
+ *(s + l - 3) = '\0';
+ }
+}
+
+static void
chktar(struct header *h)
{
char tmp[8], *err;
@@ -407,6 +428,7 @@ xt(int argc, char *argv[], int (*fn)(char *, ssize_t, char[BLKSIZ]))
(int)sizeof(h-prefix), h-prefix);
snprintf(fname + n, sizeof(fname) - n, %.*s,
(int)sizeof(h-name), h-name);
+ sanitizepath(fname);
if ((size = strtol(h-size, p, 8)) 0 || *p != '\0')
eprintf(strtol %s: invalid number\n, h-size);
--
2.1.4
From de06580d3227fc662fc89da9a922c5fabb9d792b Mon Sep 17 00:00:00 2001
From: Nick White g...@njw.name
Date: Fri, 5 Jun 2015 18:22:35 +0100
Subject: [PATCH 2/2] Strip leading ./ from path names and don't try to extract
'.' directory
There was a bug where a tarball created using '.' or './' would not
be extracted, because tar would attempt to remove the '.' directory
before extracting.
E.g.
; tar -c . ../t.tar
; tar -x ../t.tar
tar: remove ./: Invalid argument
This fixes that by ignoring a '.' directory when extracting.
Also strip any leading './'s, so './././././' will be sanitized to
to '', which will be ignored. This made the above fix easier, but
also arguably improves the interface, as tar -t myfile t.tar
works whether myfile was named 'myfile' or './myfile' in the
tarball.
---
tar.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/tar.c b/tar.c
index 0bd3fcf..7bc9687 100644
--- a/tar.c
+++ b/tar.c
@@ -367,7 +367,7 @@ sanitizepath(char *p)
/* Strip leading '/' characters */
while(*p == '/') {
l = strlen(p);
- memmove(p, p+1, l - 1);
+ memmove(p, p + 1, l - 1);
*(p + l - 1) = '\0';
}
@@ -377,6 +377,13 @@ sanitizepath(char *p)
memmove(s, s + 3, l - 3);
*(s + l - 3) = '\0';
}
+
+ /* Strip leading './'s */
+ while(!strncmp(p, ./, 2)) {
+ l = strlen(p);
+ memmove(p, p + 2, l - 2);
+ *(p + l - 2) = '\0';
+ }
}
static void
@@ -430,6 +437,10 @@ xt(int argc, char *argv[], int (*fn)(char *, ssize_t, char[BLKSIZ]))
(int)sizeof(h-name), h-name);
sanitizepath(fname);
+ if(fname[0] == '\0' ||
+ (fname[0] == '.' fname[1] == '\0'))
+ continue;
+
if ((size = strtol(h-size, p, 8)) 0 || *p != '\0')
eprintf(strtol %s: invalid number\n, h-size);
--
2.1.4
signature.asc
Description: Digital signature
[hackers] [sbase] tar: fix bug when extracting a tarball created using '.' as base path
I found this bug when fiddling about with tar today. See the commit
message for details. Note that the patch depends on the earlier
patch in this thread (as it adds a section to sanitizepath), but
that patch is good for your master too, I reckon ;-) It could be
split out easily, if you disagreed.
Nick
From de06580d3227fc662fc89da9a922c5fabb9d792b Mon Sep 17 00:00:00 2001
From: Nick White g...@njw.name
Date: Fri, 5 Jun 2015 18:22:35 +0100
Subject: [PATCH] Strip leading ./ from path names and don't try to extract '.'
directory
There was a bug where a tarball created using '.' or './' would not
be extracted, because tar would attempt to remove the '.' directory
before extracting.
E.g.
; tar -c . ../t.tar
; tar -x ../t.tar
tar: remove ./: Invalid argument
This fixes that by ignoring a '.' directory when extracting.
Also strip any leading './'s, so './././././' will be sanitized to
to '', which will be ignored. This made the above fix easier, but
also arguably improves the interface, as tar -t myfile t.tar
works whether myfile was named 'myfile' or './myfile' in the
tarball.
---
tar.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/tar.c b/tar.c
index 0bd3fcf..7bc9687 100644
--- a/tar.c
+++ b/tar.c
@@ -367,7 +367,7 @@ sanitizepath(char *p)
/* Strip leading '/' characters */
while(*p == '/') {
l = strlen(p);
- memmove(p, p+1, l - 1);
+ memmove(p, p + 1, l - 1);
*(p + l - 1) = '\0';
}
@@ -377,6 +377,13 @@ sanitizepath(char *p)
memmove(s, s + 3, l - 3);
*(s + l - 3) = '\0';
}
+
+ /* Strip leading './'s */
+ while(!strncmp(p, ./, 2)) {
+ l = strlen(p);
+ memmove(p, p + 2, l - 2);
+ *(p + l - 2) = '\0';
+ }
}
static void
@@ -430,6 +437,10 @@ xt(int argc, char *argv[], int (*fn)(char *, ssize_t, char[BLKSIZ]))
(int)sizeof(h-name), h-name);
sanitizepath(fname);
+ if(fname[0] == '\0' ||
+ (fname[0] == '.' fname[1] == '\0'))
+ continue;
+
if ((size = strtol(h-size, p, 8)) 0 || *p != '\0')
eprintf(strtol %s: invalid number\n, h-size);
--
2.1.4
signature.asc
Description: Digital signature
[hackers] [sbase] Making tar safer (was [dev] Miscellaneous sbase issues)
This is related to a patch I posted on dev@, and plan to improve, so hopefully it fits in to what you planned for hackers@. If not do berate me. Quoth Dimitris Papastamos: On Mon, Apr 27, 2015 at 08:12:42PM +0100, Nick wrote: One thing the patch doesn't cover is an archive using a symlink to somewhere like ../../ and then putting a file in symlink/newfile (hence sending it to ../../newfile). I only thought of that when reading the bsdtar manpage[0]. I'm not sure what the best behaviour is in that case. Some options: ... 3) Refuse to create any file following a symlink (this is the default behaviour of bsdtar) ... I am not sure what the proper approach is. Option 3) sounds pretty safe as a starting point. Quoth Truls Becken: +1 for option 3) Why would anybody want to trust somebody that creates malicious archives like that? A symlink in an archive should just be a symlink, nothing more. Yeah. I didn't like option 3 initially, as I imagined archives being created which included lots of complex symlink stuff that was important to replicate, but actually any non-malicious tar should use a canonical file path, and not a symlink one, obviously. I should double-check our tar implementation does that. But yes, I shall write up a patch implementing option 3 shortly. Sorry for the delay. It's nice, once this is done our tar should be the most secure implementation there is. As I mentioned previously, bsdtar supposedly does option 3, but the code is littered with FIXMEs, so I'm not convinced that it is solid. But with this in place, and the previous stuff stripping path traversal stuff, all the attacks I know of are nicely defended against. Can any creative thinkers imagine other ways to screw someone using a tar archive? Nick signature.asc Description: Digital signature
