Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
Hi Pavel, On Nov 11, 2007 2:44 PM, Pavel Machek <[EMAIL PROTECTED]> wrote: > Hi! > > > > A Smack Rule in an "egrep" format is: > > > > > > "^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*\n" > > Perhaps you should make it space, not 'space or tab', and only allow > lowercase permissions? That way, parser will be slightly simpler, and > you'll still have a chance to use 'R' as 'slightly different r'. > Thanks for your care about this. It seems not a lot of people have noticed, but to stop any objections not related to the core smack code, Casey decided to let the parsing be done in a user-space utility that sends the rules to the kernel in a predefined strict format. You can find how the whole story in the smackv11 announcement here: http://article.gmane.org/gmane.linux.kernel.lsm/4463 Regards, -- Ahmed S. Darwish Homepage: http://darwish.07.googlepages.com Blog: http://darwish-07.blogspot.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
Hi! > > A Smack Rule in an "egrep" format is: > > > > "^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*\n" Perhaps you should make it space, not 'space or tab', and only allow lowercase permissions? That way, parser will be slightly simpler, and you'll still have a chance to use 'R' as 'slightly different r'. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
Hi! A Smack Rule in an egrep format is: ^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*\n Perhaps you should make it space, not 'space or tab', and only allow lowercase permissions? That way, parser will be slightly simpler, and you'll still have a chance to use 'R' as 'slightly different r'. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
Hi Pavel, On Nov 11, 2007 2:44 PM, Pavel Machek [EMAIL PROTECTED] wrote: Hi! A Smack Rule in an egrep format is: ^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*\n Perhaps you should make it space, not 'space or tab', and only allow lowercase permissions? That way, parser will be slightly simpler, and you'll still have a chance to use 'R' as 'slightly different r'. Thanks for your care about this. It seems not a lot of people have noticed, but to stop any objections not related to the core smack code, Casey decided to let the parsing be done in a user-space utility that sends the rules to the kernel in a predefined strict format. You can find how the whole story in the smackv11 announcement here: http://article.gmane.org/gmane.linux.kernel.lsm/4463 Regards, -- Ahmed S. Darwish Homepage: http://darwish.07.googlepages.com Blog: http://darwish-07.blogspot.com - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
On Nov 10, 2007 7:05 PM, Jakob Oestergaard <[EMAIL PROTECTED]> wrote: > ... > > I've double-checked the code for any possible off-by-one/overflow > > errors. > ... > > Two things caught my eye. > > ... > > + case bol: > > + case subject: > > + if (*label_len >= SMK_MAXLEN) > > + goto out; > > + subjectstr[(*label_len)++] = data[i]; > > Why is the '>' necessary? Could it happen that you had incremented past the > point of equality? > > If that could not happen, then in my oppinion '>=' is very misleading when > '==' > is really what is needed. > Indeed, you're absolutely right. > ... > > + case object: > > + if (*prevstate == blank) { > > + subjectstr[*label_len] = '\0'; > > + *label_len = 0; > > + } > > I wonder why it is valid to uncritically use the already incremented label_len > here, without checking its value (like is done above). > > It seems strangely asymmetrical. I'm not saying it's wrong, because there may > be a subtle reason as to why it's not, but if that's the case then I think > that > subtle reason should be documented with a comment. > Maximum value label_len could reach is "SMK_MAXLEN". The subjectstr and objectstr arrays are of "SMK_MAXLEN + 1" length. So I think no danger is here. Yes, this deserved a comment. > ... > > + case access: > > + if (*prevstate == blank) { > > + objectstr[*label_len] = '\0'; > > + *label_len = 0; > > + } > > Same applies here. > > / jakob > Good spots, thanks a lot for the review. Regards, -- Ahmed S. Darwish Homepage: http://darwish.07.googlepages.com Blog: http://darwish-07.blogspot.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
... > I've double-checked the code for any possible off-by-one/overflow > errors. ... Two things caught my eye. ... > + case bol: > + case subject: > + if (*label_len >= SMK_MAXLEN) > + goto out; > + subjectstr[(*label_len)++] = data[i]; Why is the '>' necessary? Could it happen that you had incremented past the point of equality? If that could not happen, then in my oppinion '>=' is very misleading when '==' is really what is needed. ... > + case object: > + if (*prevstate == blank) { > + subjectstr[*label_len] = '\0'; > + *label_len = 0; > + } I wonder why it is valid to uncritically use the already incremented label_len here, without checking its value (like is done above). It seems strangely asymmetrical. I'm not saying it's wrong, because there may be a subtle reason as to why it's not, but if that's the case then I think that subtle reason should be documented with a comment. ... > + case access: > + if (*prevstate == blank) { > + objectstr[*label_len] = '\0'; > + *label_len = 0; > + } Same applies here. -- / jakob - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
... I've double-checked the code for any possible off-by-one/overflow errors. ... Two things caught my eye. ... + case bol: + case subject: + if (*label_len = SMK_MAXLEN) + goto out; + subjectstr[(*label_len)++] = data[i]; Why is the '' necessary? Could it happen that you had incremented past the point of equality? If that could not happen, then in my oppinion '=' is very misleading when '==' is really what is needed. ... + case object: + if (*prevstate == blank) { + subjectstr[*label_len] = '\0'; + *label_len = 0; + } I wonder why it is valid to uncritically use the already incremented label_len here, without checking its value (like is done above). It seems strangely asymmetrical. I'm not saying it's wrong, because there may be a subtle reason as to why it's not, but if that's the case then I think that subtle reason should be documented with a comment. ... + case access: + if (*prevstate == blank) { + objectstr[*label_len] = '\0'; + *label_len = 0; + } Same applies here. -- / jakob - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
On Nov 10, 2007 7:05 PM, Jakob Oestergaard [EMAIL PROTECTED] wrote: ... I've double-checked the code for any possible off-by-one/overflow errors. ... Two things caught my eye. ... + case bol: + case subject: + if (*label_len = SMK_MAXLEN) + goto out; + subjectstr[(*label_len)++] = data[i]; Why is the '' necessary? Could it happen that you had incremented past the point of equality? If that could not happen, then in my oppinion '=' is very misleading when '==' is really what is needed. Indeed, you're absolutely right. ... + case object: + if (*prevstate == blank) { + subjectstr[*label_len] = '\0'; + *label_len = 0; + } I wonder why it is valid to uncritically use the already incremented label_len here, without checking its value (like is done above). It seems strangely asymmetrical. I'm not saying it's wrong, because there may be a subtle reason as to why it's not, but if that's the case then I think that subtle reason should be documented with a comment. Maximum value label_len could reach is SMK_MAXLEN. The subjectstr and objectstr arrays are of SMK_MAXLEN + 1 length. So I think no danger is here. Yes, this deserved a comment. ... + case access: + if (*prevstate == blank) { + objectstr[*label_len] = '\0'; + *label_len = 0; + } Same applies here. / jakob Good spots, thanks a lot for the review. Regards, -- Ahmed S. Darwish Homepage: http://darwish.07.googlepages.com Blog: http://darwish-07.blogspot.com - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
On Sat, Nov 03, 2007 at 06:43:06PM +0200, Ahmed S. Darwish wrote: > On Fri, Nov 02, 2007 at 01:50:55PM -0700, Casey Schaufler wrote: > > > > Still to come: > > > > - Final cleanup of smack_load_write and smack_cipso_write. > > Hi All, > > After agreeing with Casey on the "load" input grammar yesterday, here's > the final grammar and its parser (which needs more testing): > > A Smack Rule in an "egrep" format is: > > "^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*\n" > > where Subject/Object strings are in the form: > > "^[^/[:space:][:cntrl:]]{1,SMK_MAXLEN}$" > > Signed-off-by: Ahmed S. Darwish <[EMAIL PROTECTED]> > --- > Same parser with adhering Casey's concers about previous one and with using the FSM states in a more readable way. I've double-checked the code for any possible off-by-one/overflow errors. Could someone overcheck this for any possible hidden security holes. Al, please :) ? diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c index 3572ae5..c9461cb 100644 --- a/security/smack/smackfs.c +++ b/security/smack/smackfs.c @@ -25,6 +25,7 @@ #include #include #include +#include #include "smack.h" /* @@ -67,6 +68,47 @@ struct smk_list_entry *smack_list; #defineSEQ_READ_FINISHED 1 /* + * Disable concurrent writing open() operations + */ +static struct semaphore smack_write_sem; + +/* + * States for parsing /smack/load rules + */ +enum load_state { + bol = 0,/* At Beginning Of Line */ + subject = 1,/* At a "subject" token */ + object = 2,/* At an "object" token */ + access = 3,/* At an "access" token */ + newline = 4,/* At end Of Line */ + blank= 5,/* At a space or tab */ +}; + +/* + * Represent current parsing state of /smack/load. Struct + * also stores data needed between an open-release session's + * multiple write() calls + */ +static struct smack_load_state { + enum load_state state; /* Current FSM parsing state */ + enum load_state prevstate; /* Previous FSM state */ + enum load_state prevtoken; /* Handle state = prevstate = blank */ + struct smack_rule rule; /* Rule to be loaded */ + int label_len; /* Subject/Object labels length so far */ + char subject[SMK_LABELLEN]; /* Subject label */ + char object[SMK_LABELLEN]; /* Object label */ +} *load_state; + +/* + * Rule's tokens separators are spaces and tabs only + */ +static inline int isblank(char c) +{ + return (c == ' ' || c == '\t'); +} + + +/* * Seq_file read operations for /smack/load */ @@ -131,12 +173,43 @@ static struct seq_operations load_seq_ops = { * @inode: inode structure representing file * @file: "load" file pointer * - * Connect our load_seq_* operations with /smack/load - * file_operations + * For reading, use load_seq_* seq_file reading operations. + * For writing, prepare a load_state struct to parse + * incoming rules. */ static int smk_open_load(struct inode *inode, struct file *file) { - return seq_open(file, _seq_ops); + if ((file->f_flags & O_ACCMODE) == O_RDONLY) + return seq_open(file, _seq_ops); + + if (down_interruptible(_write_sem)) + return -ERESTARTSYS; + + load_state = kzalloc(sizeof(struct smack_load_state), GFP_KERNEL); + if (!load_state) + return -ENOMEM; + + return 0; +} + +/** + * smk_release_load - release() for /smack/load + * @inode: inode structure representing file + * @file: "load" file pointer + * + * For a reading session, use the seq_file release + * implementation. + * Otherwise, we are at the end of a writing session so + * clean everything up. + */ +static int smk_release_load(struct inode *inode, struct file *file) +{ + if ((file->f_flags & O_ACCMODE) == O_RDONLY) + return seq_release(inode, file); + + kfree(load_state); + up(_write_sem); + return 0; } /** @@ -174,7 +247,6 @@ static void smk_set_access(struct smack_rule *srp) return; } - /** * smk_write_load - write() for /smack/load * @filp: file pointer, not actually used @@ -182,19 +254,26 @@ static void smk_set_access(struct smack_rule *srp) * @count: bytes sent * @ppos: where to start * - * Returns number of bytes written or error code, as appropriate + * Parse smack rules in below extended regex format: + * "^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*$" + * Where Subject/Object are: "^[^/[:space:][:cntrl:]]{1,SMK_MAXLEN}$" + * + * Handle defragmented rules over several write calls using the + * load_state structure. */ static ssize_t smk_write_load(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - struct smack_rule rule; - ssize_t rc = count; + struct smack_rule *rule = _state->rule;
[PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
On Sat, Nov 03, 2007 at 06:43:06PM +0200, Ahmed S. Darwish wrote: On Fri, Nov 02, 2007 at 01:50:55PM -0700, Casey Schaufler wrote: Still to come: - Final cleanup of smack_load_write and smack_cipso_write. Hi All, After agreeing with Casey on the load input grammar yesterday, here's the final grammar and its parser (which needs more testing): A Smack Rule in an egrep format is: ^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*\n where Subject/Object strings are in the form: ^[^/[:space:][:cntrl:]]{1,SMK_MAXLEN}$ Signed-off-by: Ahmed S. Darwish [EMAIL PROTECTED] --- Same parser with adhering Casey's concers about previous one and with using the FSM states in a more readable way. I've double-checked the code for any possible off-by-one/overflow errors. Could someone overcheck this for any possible hidden security holes. Al, please :) ? diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c index 3572ae5..c9461cb 100644 --- a/security/smack/smackfs.c +++ b/security/smack/smackfs.c @@ -25,6 +25,7 @@ #include net/netlabel.h #include net/cipso_ipv4.h #include linux/seq_file.h +#include linux/ctype.h #include smack.h /* @@ -67,6 +68,47 @@ struct smk_list_entry *smack_list; #defineSEQ_READ_FINISHED 1 /* + * Disable concurrent writing open() operations + */ +static struct semaphore smack_write_sem; + +/* + * States for parsing /smack/load rules + */ +enum load_state { + bol = 0,/* At Beginning Of Line */ + subject = 1,/* At a subject token */ + object = 2,/* At an object token */ + access = 3,/* At an access token */ + newline = 4,/* At end Of Line */ + blank= 5,/* At a space or tab */ +}; + +/* + * Represent current parsing state of /smack/load. Struct + * also stores data needed between an open-release session's + * multiple write() calls + */ +static struct smack_load_state { + enum load_state state; /* Current FSM parsing state */ + enum load_state prevstate; /* Previous FSM state */ + enum load_state prevtoken; /* Handle state = prevstate = blank */ + struct smack_rule rule; /* Rule to be loaded */ + int label_len; /* Subject/Object labels length so far */ + char subject[SMK_LABELLEN]; /* Subject label */ + char object[SMK_LABELLEN]; /* Object label */ +} *load_state; + +/* + * Rule's tokens separators are spaces and tabs only + */ +static inline int isblank(char c) +{ + return (c == ' ' || c == '\t'); +} + + +/* * Seq_file read operations for /smack/load */ @@ -131,12 +173,43 @@ static struct seq_operations load_seq_ops = { * @inode: inode structure representing file * @file: load file pointer * - * Connect our load_seq_* operations with /smack/load - * file_operations + * For reading, use load_seq_* seq_file reading operations. + * For writing, prepare a load_state struct to parse + * incoming rules. */ static int smk_open_load(struct inode *inode, struct file *file) { - return seq_open(file, load_seq_ops); + if ((file-f_flags O_ACCMODE) == O_RDONLY) + return seq_open(file, load_seq_ops); + + if (down_interruptible(smack_write_sem)) + return -ERESTARTSYS; + + load_state = kzalloc(sizeof(struct smack_load_state), GFP_KERNEL); + if (!load_state) + return -ENOMEM; + + return 0; +} + +/** + * smk_release_load - release() for /smack/load + * @inode: inode structure representing file + * @file: load file pointer + * + * For a reading session, use the seq_file release + * implementation. + * Otherwise, we are at the end of a writing session so + * clean everything up. + */ +static int smk_release_load(struct inode *inode, struct file *file) +{ + if ((file-f_flags O_ACCMODE) == O_RDONLY) + return seq_release(inode, file); + + kfree(load_state); + up(smack_write_sem); + return 0; } /** @@ -174,7 +247,6 @@ static void smk_set_access(struct smack_rule *srp) return; } - /** * smk_write_load - write() for /smack/load * @filp: file pointer, not actually used @@ -182,19 +254,26 @@ static void smk_set_access(struct smack_rule *srp) * @count: bytes sent * @ppos: where to start * - * Returns number of bytes written or error code, as appropriate + * Parse smack rules in below extended regex format: + * ^[:space:]*Subject[:space:]+Object[:space:]+[rwxaRWXA-]+[:space:]*$ + * Where Subject/Object are: ^[^/[:space:][:cntrl:]]{1,SMK_MAXLEN}$ + * + * Handle defragmented rules over several write calls using the + * load_state structure. */ static ssize_t smk_write_load(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - struct smack_rule rule; - ssize_t rc = count; + struct smack_rule