-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bugfixes: initialize asg level to avoid randomized access rights and fix buffer overrun #52
base: master
Are you sure you want to change the base?
Conversation
Duh. Thanks a lot! Why is static analysis not finding this? It should. |
I have not seen any compiler warnings about this either. |
Maybe because Also, as I read it, |
607: if((asg=strtok(NULL," \t\n"))) { // ASG / ASL
608: if((asl=strtok(NULL," \t\n")) &&
609: (sscanf(asl,"%d",&lev)!=1)) lev=1;
610: } else {
611: asg=(char*)default_group;
612: lev=1;
613: } The inner Given that the default in the code above is ASL 1, I suspect this PR should be changed to set Dirk, can you look at the pvlist file where you were seeing this issue and confirm that the first PV name pattern in it gives an ASG name but not an ASL value? That's the only way I can see |
None of my pvlist files use ASL. I usually use two ASGs, DEFAULT and WRITE, so my pvlist files have lines like |
Good, all our pvlist patterns that name an ASG also give an ASL, which explains why we haven't seen this problem here. We do make extensive use of ASL (or more accurately our pvlist files have patterns with both settings), but I don't know how important the choice is to our protection design. I'm not certain that I understand how ASL is used in the gateway, but we do have ASGs with separate rules for level 0 and level 1 (not sure if higher level numers are allowed here or not, they aren't by the IOC) so this lets us set different access rules for different users on the same group or records. |
docs/GATEWAY.pvlist also says the default is 1:
I will change it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now. I won't click the "Approve and Run" button since the tests here failed last time I did that.
Initializing to while(fgets(inbuf,sizeof(inbuf),fd)) {
++line;
pattern=rname=hname=NULL;
lev = 1; /* <-- add */ imo. a better long term strategy would be to move these variable definitions into the loop. I think this would more clearly represent the intent to readers, and to static analysis tools. |
@@ -391,7 +391,7 @@ gateAsEntry* gateAs::findEntryInList(const char* pv, gateAsList& list) const | |||
int len = (int) strlen(pv); | |||
#ifdef USE_PCRE | |||
pi->substrings=pcre_exec(pi->pat_buff, NULL, | |||
pv, len, 0, PCRE_ANCHORED, pi->ovector, 30); | |||
pv, len, 0, PCRE_ANCHORED, pi->ovector, pi->ovecsize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-and-paste bug I had introduced in or before 2007.
Access was sometimes not granted when it should be. Access changed randomly upon gateway restarts.
Generating an AS report showed random
ASL
values. Depending on it sign, access was granted or not.This fix initializes the access level to
0
.