Question

Consider the following general code for allowing access to a resource:

Answer 1

Answer:

a) The code allows access even when  IsAccessAllowed(...) method fails.

b) Either use If

(dwRet = ACCESS_ALLOWED)

or use

if (dwRet == NO_ERROR)

to avoid flaw

Explanation:

Lets first see what the code chunk does:

DWORD dwRet = IsAccessAllowed(...);

if (dwRet == ERROR_ACCESS_DENIED) {

// Security check failed.

// Inform user that access is denied.

} else {

// Security check OK.

}

In the given code, DWORD is basically a data type for double word type integers and this is defined in windows.h

So there is DWORD type variable dwRet that is assigned a method calls.

The method is IsAccessAllowed() which checks if the access is allowed to user.

if (dwRet == ERROR_ACCESS_DENIED) condition basically checks if the value of DWORD type variable dwRet is equal to ERROR_ACCESS_DENIED

If this condition evaluates to true then the security checks fails and user is informed via some message or action that the access is denied.  But when the if condition evaluates to false then the else part executes which allows access.

So basically this chunk of code checks if the error ERROR_ACCESS_DENIED is returned.

Now the flaw in this program is what if the method IsAccessAllowed() by any reason. The reasons can be system failure or the memory failure. In memory failure case for example, the system returns out of memory error. So this means that the error is not  ERROR_ACCESS_DENIED. Instead it is out of memory error. So in such a case the user is allowed access as the if condition evaluates to false and else part executes. So if any other error is produced due to some reason like mentioned above, then user has unrestricted access.

This shows that the doe should not check for the failure or rely on checking ERROR_ACCESS_DENIED to allow access but instead it should check for success. Code should only give access privilege if access is allowed successfully or no error is produced.

So to avoid this flaw the code is altered as:

DWORD dwRet = IsAccessAllowed(...);

If (dwRet = ACCESS_ALLOWED) {

//Security check OK.

} else {

//Security check failed.

//Inform user that access is denied.

}

This will only allow access if ACCESS_ALLOWED evaluates to true and success is checked instead of failure here

You can also alter the if condition as:

If (dwRet = No_Error)

or

If (dwRet = 0)

The above if conditions checks if the access is allowed or if no error is produced. Only then it will allowed access otherwise not. So the access check is a success is checked first and failure (for any reason). The user is allowed access only if there is no error otherwise user is not allowed access.