Unsafe Object Binding in Checkmarx scan

Question

I have tried adding null check and try catch blocks but I am not able to solve this issue.

Error : The DeleteConfirmed at VCSSource/Web/Controllers/EnvController.cs in line 180 may unintentionally allow setting the value of SaveChanges in DeleteConfirmed, in the object VCSSource/Web/Controllers/EnvController.cs at line 180

Code :

[HttpPost, ActionName("Delete")]
[ValidateAntiForgeryToken]
public ActionResult DeleteConfirmed(int id)
 {
     if (id > 0) // fix for CheckMarx : Unsafe Object Binding
        {
           ENV eNV = db.ENVs.Find(id);
           ENV eNV_del = db.ENVs.Remove(eNV);              
            try
              {
                  if (eNV_del != null && eNV_del.ENV_NM.Length > 0) {
                       db.SaveChanges();
                      }                   
                return RedirectToAction("Index");
              }
            catch (DataException ex)
              {
                   throw ex;
              }
            }
     else
       {
          return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
       }
   }

What are the constraints on id? Can it be negative? It wants you to validate the input. If truly any id is valid then you can mark it not exploitable. Checkmarx isn't really an authority, it just makes well educated assumptions. — , Dec 03 '19 at 16:16
@JoshWilliard , Thank you for the information . I have added the validation but the issue remains the same. — Jay, Dec 04 '19 at 05:52
I suspect id to be an input parameter. If yes, Checkmarx see a direct reference to the DB and could mark it as Unsafe Object Binding. — SPoint, Dec 05 '19 at 13:31

score 2 · Accepted Answer · answered Dec 10 '19 at 12:42

2

I believe the issue that it is highlighting is that you take an integer ID in as input and directly act upon that ID without any further validation (i.e. anyone can hit this endpoint with that ID and delete that item).

Part of this issue is that it is then trivial for an attacker to iterate over integers starting from 1 and delete anything in ENV.

You need to either mark this as 'not exploitable' as you understand how this endpoint is locked down within the application or potentially introduce a secondary method of looking up this object (e.g. a GUID identifier on the table) and have the method take the GUID identifier instead of the integer primary key.

answered Dec 10 '19 at 12:42

Paddy

33,309
15
79
114

1

You should add an authorization check before. This should make checkmarx happy – SPoint Dec 12 '19 at 08:57
I am facing the same issue, but have a GUID already not a integer value. In this case how should I solve this issue if i need to – Darshani Jayasekara Jul 02 '21 at 06:21
You need to ask that as a separate question with details please. – Paddy Jul 03 '21 at 07:07

Unsafe Object Binding in Checkmarx scan

1 Answers1